465672 - [jsd] jsdIDebuggerService onError or onDebug is broken.

Status: REOPENED

(Firefox :: General, defect)

Description

[John J. Barton]

return true from onError no longer triggers onDebug in FF3.0.4

1. Install Firebug 1.4X from http://getfirebug.com/releases
2. Open Firebug icon (upper left), Open trace console
3. Options ERRORS  and FBS_ERRORS
4. Download /fbug/tests/issues/issue1268 from http://code.google.com/p/fbug/source/checkout
5. run http://localhost:63636/EclipseWorkspace/fbug/tests/issues/issue1268/throwNewError.html
6. look at the Trace log. onError is called and trace indicates that it returned true. but onDebug is not called.

This used to work and its the basis for Firebug's stack trace on error.

Comment 1

[John J. Barton]

can't reproduce now

Comment 2

[Ryan Johnson]

Using Firefox 3.1 and the latest 3.2 alpha build I can confirm that stack traces are still not present from user / developer generated errors. This is a major issue for JS development, please consider re-opening.

Comment 3

[John J. Barton]

How are you seeing the errors messages?

Comment 4

[Ryan Johnson]

I am using firebug 1.3.0b7, when a user / developer generated error is thrown, I see a red X, followed by the error message on a single line, and nothing more. Internal (value undefined, etc) errors still have the stack trace toggle to the left of the error.

Comment 5

[John J. Barton]

Firebug issue 1260 has a test case that demos this long standing bug.
http://code.google.com/p/fbug/issues/detail?id=1260

Tested with FF 3.5.2 and Firebug 1.5a21 or 1.4X2

What is happening is that firebug is returning a signal from onError to cause jsd to call onDebug, but that signal is being ignored. Then firebug cannot create the call stack. (the stack is not available in onError).

I recall the onError is called in jsd_xpc.cpp around line 575.

I've had the idea to try to do more about error handling in onDebug, but its not reliable because of this bug.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So I'm missing some context here.  Is this a Gecko regression?  If not, can you point me to the exact code Firebug is running here?

Comment 7

[John J. Barton]

(In reply to comment #6)
> So I'm missing some context here.  Is this a Gecko regression?  

I don't know. It has worked and does work sometimes. My guess is that there are two code paths: one works, one does not. (or more obviously).

> If not, can you
> point me to the exact code Firebug is running here?

The onError hook is:
http://code.google.com/p/fbug/source/browse/branches/firebug1.5/components/firebug-service.js#1230

and the onDebug which is supposed to be called is
http://code.google.com/p/fbug/source/browse/branches/firebug1.5/components/firebug-service.js#1079

Comment 8

[Boris Zbarsky [:bzbarsky]]

Is comment 0 incorrect then?  The jsd documentation says that debugHook should be called when errorHook returns false, not true...

I assume you have a case where the errorHook returns false but the debugHook is not being called?  And that in this situation you're not hitting the "Error in hook" case?

Comment 9

[John J. Barton]

If Console showStackTrace is true, (default), then the onError always return false. I count the times we take the false path and decrement when we land in onDebug. The number goes up. (And I don't see the stack trace or tracing output from onDebug). 

http://code.google.com/p/fbug/source/browse/branches/firebug1.5/components/firebug-service.js#1265

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm.  I can't seem to locate the test file from comment 0 (either in my local svn checkout of firebug nor in google code).  Any chance the relevant HTML file can just be attached to this bug?  If so, does doing the steps from comment 0 on that attachment still show the problem?

I read over the onerror/ondebug code in the jsdservice and it looks like it should work at first glance, so something odd is going on.  Would be nice to have a debuggable testcase... ;)

Comment 11

[John J. Barton]

Attachment: test case from firebug issue

Comment 12

[John J. Barton]

The test case referred to in comment 5 is attached, maybe it helps?

Comment 13

[John J. Barton]

I hit this once again, on
http://getfirebug.com/tests/issues/bugzilla/11240/EventHandlerErrorMessages.html
Hit Click Me to trigger the exception.

To investigate I set a breakpoint in jsd_xpc.cpp on the line:
hook->OnError (nsDependentCString(message), fileName, line, pos, flags, errnum, val, &rval);

After we hit the jsd.onError, if we have Firebug > Console > Show StackTrace true, then we return to jsd_DebugErrorHook and the switch(errorReporter) goes to branch JSD_ERROR_REPORTER_DEBUG. We then call for the hook (onDebug) and jsd_CallExecutionHook calls jsd_NewThreadState(). That function fails.

>	jsd3250.dll!jsd_NewThreadState(JSDContext *, JSContext *, JSStackFrame *)  Line 121	C
 	jsd3250.dll!jsd_CallExecutionHook(JSDContext *, JSContext *, unsigned int, unsigned int (JSDContext *, JSDThreadState *, unsigned int, void *, int *)*, void *, int *)  Line 165	C
 	jsd3250.dll!jsd_DebugErrorHook(JSContext *, const char *, JSErrorReport *, void *)  Line 373	C

And fails because it decides that there is no JS stack frame, which of course is ridiculous since we got here from a |throw| in a JS stack frame.

Comment 14

[John J. Barton]

When we call js_ReportUncaughtException(JSContext *cx), cx->fp is 0x0, so I guess from the function name that this guy only gets called after the stack has unwound from the throw. So uncaught exceptions will always have no stack and they will never go through the onDebug path.

Comment 15

[John J. Barton]

Ok I record the file/line/window in the onThrow() handler and if I get in to onError with an jsdIErrorHook.REPORT_EXCEPTION, then I report the file and line. So the error message from bug 11240 goes from 
uncaught exception: i'm dubious
with no file or line info to a link to the source code line plus the file and line number.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> When we call js_ReportUncaughtException(JSContext *cx), cx->fp is 0x0

Yeah, by the time js_ReportUncaughtException is happening, that means we're returning completely out of js code.  That said, did cx->fp always use to be null by this point?  If so, this might just have never worked.  Brendan, an idea how this is possibly supposed to work?

Comment 17

[Brendan Eich [:brendan]]

This has been the way it is for a long time, going back way into cvs history (rev 3.14 of jsapi.c). See also bug 243869.

/be

Comment 18

[Brendan Eich [:brendan]]

My "This" was referring to LAST_FRAME_CHECKS or its open-coded predecessor. But something else may have regressed to expose this bug. What?

/be

Comment 19

[John J. Barton]

But it seems to me that cx->fp is correct: the exception was uncaught, we unwound and left JS. 

We could call onDebug anyway, with an empty frame, or just document the onError won't call onDebug on this case. I think the latter.

A different question is how to give the uncaught exception useful information. Apparently the line information is transported to onError comes from creating Error() objects, the correct line number arrives from |throw new Error("ff")|. This relies on developers doing the right thing, which is pretty much not a good idea for error handling.

On the other hand, recording call stacks on every throw, just in case they might be uncaught is not an attractive option.

Recording the line information onThrow is a decent compromise. The main problem is knowing when to trust the result: if we arrive in onError and onThrow has recorded some line info, does it belong to us? I think just looking for onError calls with bad info (no file or line) is decent.  

Now that I understand this issue it is not as important as other error problems.

Comment 20

[John J. Barton]

(In reply to comment #19)
> On the other hand, recording call stacks on every throw, just in case they
> might be uncaught is not an attractive option.

I was not thinking clearly here. 

Here is what I imagine: At a throw we will enter a code path in the engine that will go only two ways: either we hit a catch block or we have an uncaught exception. Specifically we do not have arbitrary or any JS code running between the throw and next operation. We can have a record of the complete call stack at the uncaught exception and in fact we could cause the engine to break at the point of the throw which is what developers really want.

Of course it may be hard to get there if the internal stack representation is being discarded as the exception is propagating and if creating and resetting copies of this stack is not practical.

> 
> Now that I understand this issue it is not as important as other error
> problems.

The part that is less important is just the onDebug trigger. Two aspects of uncaught exception are very important: 1) devs want the break on uncaught exception, it is the exact important case, 2) the xpconnect-fails-to-report problem involves uncaught exceptions.