BugSack

BugSack

7M Downloads

BugGrabber and New Blizzard handler interface in Retail (error forwarding feature request)

2072 opened this issue ยท 8 comments

commented

Describe the problem

This is for BugGrabber (I couldn't find its repository)
In the retail version of wow there is a new internal interface to manage Lua errors with logging and stuff.

See below (code from 11.1.059538 in _retail_\BlizzardInterfaceCode\Interface\AddOns\Blizzard_ScriptError\Blizzard_ScriptError.lua)

local function GetErrorData()
	-- Example of how debug stack level is calculated
	-- Current stack: [1, 2, 3, 4, 5] (current function is at 1, total current height is 5)
	-- Stack at time of error: [1, 2] (these are currently now index 4 and 5, but at the time of error the stack height is 2)
	-- To calcuate the level to debug (4): curentStackHeight - (errorStackHeight - 1) = 5 - (2 - 1) = 4
	local currentStackHeight = GetCallstackHeight();
	local errorCallStackHeight = GetErrorCallstackHeight();
	local errorStackOffset = errorCallStackHeight and (errorCallStackHeight - 1);
	local debugStackLevel = currentStackHeight - (errorStackOffset or 0);
	local skipFunctionsAndUserdata = true;

	local stack = debugstack(debugStackLevel);
	local locals = debuglocals(debugStackLevel, skipFunctionsAndUserdata);
	locals = string.gsub(locals, "|([kK])", "%1");
	
	return stack, locals;
end

local warningHandlers = {};
local errorHandlers = {};

function AddLuaWarningHandler(handler)
	assert(issecure());
	table.insert(warningHandlers, handler);
end

function AddLuaErrorHandler(handler)
	assert(issecure());
	table.insert(errorHandlers, handler);
end

function HandleLuaWarning(warnType, warningMessage)
	local stack, locals = GetErrorData();
	C_Log.LogMessage(Enum.LogPriority.Warning, string.format("Lua Warning: message=%s", warningMessage));

	for index, handler in ipairs(warningHandlers) do
		pcall(handler, warnType, warningMessage, stack, locals);
	end
end

local function HandleLuaError(errorMessage)
	local stack, locals = GetErrorData();
	local formattedMessage = string.format("Lua Error: %s\n%s", errorMessage, stack);
	addframetext(formattedMessage);
	
	C_Log.LogMessage(Enum.LogPriority.Error, formattedMessage);

	for index, handler in ipairs(errorHandlers) do
		pcall(handler, errorMessage, stack, locals);
	end

	if ProcessExceptionClient then
		ProcessExceptionClient(string.format("%sLocals: %s", errorMessage or "", locals or ""), errorMessage);
	end
end

seterrorhandler(HandleLuaError);

As you can see there is internal logging involved and stuff and they made their handler local so it's not possible to call it manually.

In Decursive which is using the embedable version of Buggrabber I used to forward non-Decursive errors to the original handler, so that users can see them if they don't have BugSack but now having Decursive without BugSack means that no other error can be seen by the user or Blizzard since they now have internal logging it seems...

It would be wise if Buggrabber could forward to the original handler instead of keeping it all for itself.

At the very least could you add an API to get the original error handler back? So I can use it to forward them from Decursive?

What steps will reproduce the problem?

N/A

Consider attaching a screenshot below to help describe your issue (Attach directly, do not link to other websites)

N/A

What version of the addon are you using? (Stating 'latest' is not useful)

v11.0.2 BugGrabber and v11.0.3 BugSack

Do you have an error log of what happened?

N/A

Any additional information? (example: WoW language if not English)

N/A

commented

If you want a reference to the original error handler, you can do this.

BugGrabber.lua (around line 255 or so.) call geterrorhandler() to get a reference to the current error handler (the local HandleLuaError).

-- Error handler
local grabError
local originalErrorHandler = geterrorhandler()
function addon:GetOriginalErrorHandler() return originalErrorHandler end

function addon:SendToOriginalErrorHandler(errorMessage) originalErrorHandler(errorMessage) end

Then you can call BugGrabber:GetOriginalErrorHandler()("your error text") to pass an error to the original error handler. Or you can do BugGrabber:SendToOriginalErrorHandler("your error text") to do the same thing without the extra function call.

This is not a "perfect" solution but it works for what you want.

commented

@ChrisKader in what version of Buggrabber do you see this? There is no such code in the current version.

edit, you mean @funkydude could do this, yes indeed that would be a better than nothing solution.

commented

I mean you could add this code to your embeded one.

commented

Thanks for the suggestion! I just save the original error handler before loading the embeded version of Buggrabber. I'll need to remember to disable that once it's fixed here.

commented

I'm not seeing what needs fixed here, or I don't understand the issue you have. I don't think the original intention for allowing embedding was ever for addons to try to forward errors to the Blizzard handler.

If this was the original intention, we'd be hard disabling the embedded version if the user is using the Blizz one (just like we do if the user has BugGrabber proper installed), and we don't.

Realistically I don't think anyone is actually using the Blizz one as I cannot think of anything more intrusive than error windows spamming you during a boss fight.

commented

Ok let me try to explain this in more details then.
It seems that Blizzard is now using a more advanced error handling mechanism with advanced logging that they probably use internally to find out about their own issues and maybe do some QA.
The code I posted above also shows that they do support multiple error handlers now with AddLuaErrorHandler and AddLuaWarningHandler.

The Blizzard Lua error popup has been disabled by default for several years now (the only way to turn it back on is to change the ScriptErrors cvar)

So my idea is that instead of depriving Blizzard from this feedback whenever Buggrabber or an add-on embedding it is installed, we could just forward (given that the popup is disabled it won't annoy the user and give the feedback they want to Blizzard).

I use the embedded version of Buggrabber in Decursive to create debug reports for my users to send me but, if BugSack is not installed I also forward the other errors (not related to Decursive) to the original Blizzard handler so as not to break the original behavior or selfishly hide all other Lua errors (I grab my errors and let the other errors behave like they would have if Decursive had not been installed).

Anyway I've implemented a workaround in Decursive (where I save the original handler before loading Buggrabber) but I thought it would have been better if this was the default behavior. (Since Blizzard seems to care a little more about Lua errors now and that they might be annoyed to lose this feedback whenever certain add-ons are installed).

Note that this is only true for WoW retail, they still use the old simple handler in WoW classic (but the popup is also disabled by default).

commented

given that the popup is disabled it won't annoy the user and give the feedback they want to Blizzard

Since Blizzard seems to care a little more about Lua errors now and that they might be annoyed to lose this feedback whenever certain add-ons are installed

What feedback? Blizzard doesn't care at all about errors outside of their addons.

I use the embedded version of Buggrabber in Decursive to create debug reports for my users to send me but, if BugSack is not installed I also forward the other errors

I understand the situation you are in, but from my perspective no one cares at all about errors. The only reason they are willing to install BugSack is it can collect them in a non-intrusive manner. If it didn't it would be instantly deleted.

commented

On PTR only AddLuaErrorHandler remains and it is only callable from secure code (AddLuaWarningHandler is also only usable with secure code but also does not exist on PTR).

I mainly was wanting BugGrabber to not modify seterrorhandler while also preserving the original return from geterrorhandler. BugGrabber could hook seterrorhandler so that captures when new error handlers are trying to be set and set its own again or some other kind of functionality (keeping track of each call to seterrorhandler and maybe calling each function sent in a chain versus only having one, for example). That way the original return from geterrorhandler (The error handler created by Blizzard in Blizzard_ScriptError.lua) can still be called allowing for blizzard addon logging to be handle the same. An addon like BugSack can handle hiding the defaut error frame, if desired.

BugGrabber could also allow addons to register themselves to pass a function that "reviews errors" sent to the display UI to determine if it should be sent or handled differently.