489671 - [FIX]errors from event handler not reported to nsIConsoleService if event dispatched from JavaScript

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[John J. Barton]

Errors are not being sent to the console service in FF 3.5 correctly.

Open as a web page:
http://fbug.googlecode.com/svn/tests/content/console/testErrors.html
or
http://code.google.com/p/fbug/source/browse/tests/content/console/testErrors.html

Click on the button 'Fire two errors via events'.

On FF 3.0.8 you will see two errors in the Error Console.
On FF 3.1b3 you will see zero.

Firebug relies on the nsIConsoleService for error reporting.

The example is slightly unusual: the errors are triggered by two 'click' events dispatched in succession.  If you click on other buttons on the test page you will see that they work correctly. If we can understand the source of the problem here we may not need to block 1.9.1 if we can decide this is a corner case.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Can you reproduce on b4? Not sure I understand your comment #0 - you think this blocks because ..?

Comment 2

[Nochum Sossonko [:Natch]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b5pre) Gecko/20090429 Shiretoko/3.5b5pre

Still buggy. You can use the error console as it uses the same method to retrieve errors (nsIConsoleService).

Comment 3

[Nochum Sossonko [:Natch]]

Actually, there is no error here at all in 3.5, that seems more drastic. I confirmed that the click works, but no error at all is thrown (i.e. even with a try{}catch block).

Comment 4

[Nochum Sossonko [:Natch]]

(In reply to comment #3)
err, take that back, it does throw with a try{}catch block, only the console service doesn't receive it. That's weird!

Comment 5

[John J. Barton]

(In reply to comment #1)
> Can you reproduce on b4? Not sure I understand your comment #0 - you think this
> blocks because ..?

Because we need error messages to debug web page errors? I can't understand how this would not block if it is a real bug.

Comment 6

[Boris Zbarsky [:bzbarsky]]

John, can you attach an HTML file that shows the problem to this bug (instead of pointing to text/plain output)?

Comment 7

[John J. Barton]

Attachment: copy of testErrors.html from R2739 on fbug

Does this help?

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Brendan, Sayrer: can you please evaluate for blocking? I don't think this component appears in your normal queries ...

Comment 9

[timeless]

so, i'd like to lodge a complaint. this component is not supposed to be jjb's dumping ground. (In fact, it's *my* dumping ground.)

nsIConsoleService is clearly an XPCOM interface

http://mxr.mozilla.org/mozilla-central/find?string=nsIConsoleService
/xpcom/base/nsIConsoleService.idl

serviced by an XPCOM component which means it would belong in XPCOM.

http://mxr.mozilla.org/mozilla-central/search?string=nsIConsoleService&find=&filter=%5Cbclass%5Cb.*%3A.*nsIConsoleService
/xpcom/base/nsConsoleService.h

If the problem is in DOM or XPConnect which use the interface, then the bug belongs in DOM or XPConnect respectively, which can be trivially established by looking for callers of the nsIConsoleService interface

http://mxr.mozilla.org/mozilla-central/search?string=nsIConsoleService&find=dom%2F
http://mxr.mozilla.org/mozilla-central/search?string=nsIConsoleService&find=js%2F

And if there's actually a problem w/ the js side, then someone should be able to establish a testcase which doesn't involve nsIConsoleService at all.

Lastly, one could try to use hg bisect to find the checkin which "broke" this.

Comment 10

[John J. Barton]

(In reply to comment #9)
> so, i'd like to lodge a complaint. this component is not supposed to be jjb's
> dumping ground. (In fact, it's *my* dumping ground.)

If I don't pick a component someone will complain. If I pick a component, someone will complain...

> 
> nsIConsoleService is clearly an XPCOM interface
> 
But the only reason I mentioned nsIConsoleService in the report is that I knew that if I said "jsd is not doing X", then nothing would happen. By reporting that nsIConsoleService fails I have a better chance of confirmation. I don't actually think the problem has anything to do with nsIConsoleService.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Does this help?

Yes.  The behavior changed on m-c between the 2008-10-12-02 nightly (rev ee5135f3f773) and the 2008-10-13-02 nightly (rev 163e9f2fa5c1).  Pushlog:  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ee5135f3f773&tochange=163e9f2fa5c1

Looks like a regression from bug 455436.  In this case, when we get to NS_ScriptErrorReporter (called via nsJSContext::ReportPendingException from nsJSContext::CallEventHandler after the event dispatch returns), cx->fp->script is of course non-null.  It's the script that called handleEvent.  So the code bug 455436 added to NS_ScriptErrorReporter causes the latter to return without actually reporting anything.

I think this needs to block 1.9.1.  Either the code in NS_ScriptErrorReporter needs to stop making assumptions about whether it's supposed to report the error, or we need to work around that code somehow at all callsites of JS_ReportPendingException and the like that might happen while other script is running.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #10)
> If I don't pick a component someone will complain. If I pick a component,
> someone will complain...

If you don't know, please use Core::General or Firefox::General and it will be moved appropriately.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: This might be one approach to fixing this

The basic idea is to make all the nsIScriptContext methods report exceptions (they already did except for EvaluateStringWithValue) and to make them all set aside the frame chain before doing so (some already did, some did not).  It seems to me that always setting it aside is the behavior we want; without that we get into problems as it is with inline script testcase in the patch, for example.

The patch also fixes XBL constructors set aside the frame chain before reporting errors, for similar reasons.

I didn't touch other cases of JS_ReportPendingException; at least some are safe because async (e.g. javascript: URIs) and the rest are xpconnect and workers.  The one exception is the one in nsJSNPRuntime.cpp; not sure whether that one should set the frame chain aside.

The one behavior change here is that the other caller of EvaluateStringWithValue (the first is XBL fields) is _evaluate in nsNPAPIPlugin.cpp.  jst, do you think it's ok to make this report before returning to the plugin?  As it stands, I don't know that exceptions there ever managed to report...

Comment 14

[Johnny Stenback (:jst)]

Blocking.

Comment 15

[Boris Zbarsky [:bzbarsky]]

That patch seems to cause some mochitest failures on try server.  Looking into it now.

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK, the reason for at least some of these is that the test is in fact broken: it has an event handler that got changed so that it throws exceptions now, and no one noticed because of this bug (we were eating the exceptions and never reporting them to the mochitest onerror handler).

Patch coming up with that test fixed; I've also pushed that patch to the try server.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: With the broken test fixed

Comment 18

[Johnny Stenback (:jst)]

Neil's out on vacation until the 25th last I heard, so don't wait for him to review here. Blake and I can probably cover this one, at least until Neil gets back...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 378240 [details] [diff] [review]
With the broken test fixed

Trying the other neil, then, since he last touched this test code.

This patch passes try server unit tests.

Comment 20

[[e-mail]]

Comment on attachment 378240 [details] [diff] [review]
With the broken test fixed

I don't think I've touched this test at all but the changes look sane to me.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 378240 [details] [diff] [review]
With the broken test fixed

Oh, indeed.  Those changes are enn; he just appears as both "enndeakin" and "neil" in mercurial blame... :(

So still could use review from enn when he gets back.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/a71886cfb2e4 and http://hg.mozilla.org/mozilla-central/rev/ecd3367a162a (a followup comment fix).

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.1 merge

This stuff was different enough on 1.9.1 that it's worth reviewing the branch merge separately, I think.  I'll go ahead and push it anyway, because I'm pretty sure it's correct, but please double-check it?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d78ac0da4a3f

Comment 25

[John J. Barton]

While I can verify that in
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090531 Shiretoko/3.5pre
the test button works correctly, the original code (Firebug FBTest) still fails in this version but works in 3.0.10.  Must be a another problem.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Please file, with a way to reproduce, ok?

Comment 27

[John J. Barton]

It will have to wait til mid week. I need to run under C++ debugger to figure out where control flow is going. We set a listener to wait for Firebug Console log events, then cause errors. The first error works fine. The second error gets in to our jsd.onError() handler but never appears in nsIConsoleService. 
(The failing FBTest is console/test/testErrors.js).