Closed Bug 812137 Opened 12 years ago Closed 12 years ago

Error object (nsIScriptError) for exception thrown in event handler has outerWindowID corresponding to the window the event handler is defined in, not the window the event target is in

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Windows Vista
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: Honza, Unassigned)

References

Details

(Whiteboard: [firebug-p1]) 

STR:
1) Install Firebug 1.10.6
https://getfirebug.com/releases/firebug/1.10/firebug-1.10.6.xpi

2) Open https://getfirebug.com/tests/head/console/2271/issue2271.html
and follow instructions

The error isn't visible in the Console panel -> BUG

---

The problem is that Firebug is overriding XHR onreadystatechange method and so, the top JS stack comes from firebugFrame.xul (chrome scope, part of Firebug) instead from the page itself (issue2271.html)

---

Regression Range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90cea19e27e2&tochange=a47525b93528

Honza
Boris, any tips what commit could cause this problem?

Honza
(In reply to Jan Honza Odvarko from comment #0)
> The problem is that Firebug is overriding XHR onreadystatechange method and
> so, the top JS stack comes from firebugFrame.xul (chrome scope, part of
> Firebug) instead from the page itself (issue2271.html)
Just to make this a little more clear.

It's my theory explaining why the outerWindowID (in the error object) is pointing to a XUL window coming from Firebug (which is clearly wrong) instead to the page itself (where the error happened).

Honza
My money's on bug 807226.
That was my personal tip too (and I was also thinking about bug 779048)

Honza
This is a purposeful change from bug 807226, actually.  We used to run the code on the JSContext of the event target; now we run it on the JSContext that most closely corresponds to the actual listener involved.  This just happens to be what the spec calls for too, in addition to generally, for purposes of the web, being saner.

I would argue it's more correct in the Firebug case too: the exception is not in the page in this case, but in Firebug, right?
Component: Networking: HTTP → DOM
Summary: Error object (nsIScriptError) has wrong outerWindowID → Error object (nsIScriptError) for exception thrown in event handler has outerWindowID corresponding to the window the event handler is defined in, not the window the event target is in
Oh, but Firebug calls back into page code, eh?

Ideally, in terms of specs this would involve it ending up with a new script entry point, since it really doesn't want to treat its script as part of the web page execution context.  I'm not sure how feasible that is in our current wrapper setup.  Bobby?

That said, do you have to use onreadystatechange at all?  Or would just a readystatechange listener be enough?  That is, are you trying to _filter_ readystatechange events the page gets, or just be notified of changes?
Oh, and one more thing.  Generally speaking for JS exceptions the outerWindowID is gotten in a slightly obtuse way: it points to the place where the exception ended up uncaught, no the place where it got thrown.  The main reason apparently being that JSErrorReport simply does not provide that last bit of information.  So we end up using the window whose onerror handler we're about to call, and _that_ is what the spec addresses and what we changed in bug 807226.
Blocks: 807226
Or to put it yet another way.. in the old setup, if the exception _were_ thrown in Firebug's code it would still have been reported with the window id of the web page.
(In reply to Boris Zbarsky (:bz) from comment #5)
> I would argue it's more correct in the Firebug case too: the exception is
> not in the page in this case, but in Firebug, right?
No, the exception happens on the web-page (not in Firebug)

(In reply to Boris Zbarsky (:bz) from comment #6)
> Oh, but Firebug calls back into page code, eh?
Yes. Firebug overrides the original 'onreadystatechnage' handler by its own function and when it's executed it calls the original onreadystatechnage handler.

> That said, do you have to use onreadystatechange at all?  Or would just a
> readystatechange listener be enough?  That is, are you trying to _filter_
> readystatechange events the page gets, or just be notified of changes?
One problem with the readystatechange listener I know about, is that if the original onreadystatechange handler calls abort(), listeners are confused (e.g. the xhr.responseText is not available anymore, the readyState is reset...)

Note that Firebug is using the outerWindowID to find out in which page the error actually happened (because nsIConsoleService is global). Firebug needs to know the source page to decide whether the error should be displayed for the current page or not.

If extensions can accidentally change the outerWindowID, it doesn't sound correct to me. It should always point to the origin/parent window.

In any case, is there any other solid way how to find out what is the window/page where the error is coming from?

Honza
(In reply to Boris Zbarsky (:bz) from comment #8)
> Or to put it yet another way.. in the old setup, if the exception _were_
> thrown in Firebug's code it would still have been reported with the window
> id of the web page.
Yes, precisely.

Firebug is re-throwing the exception. That's the problem.

https://github.com/firebug/firebug/blob/master/extension/content/firebug/net/spy.js#L843

So, one option would be to also pass the original outerWindowID along? Would that be possible?

Honza
> One problem with the readystatechange listener I know about

None of that answered my question.  Which is unfortunate, because answering it might let us figure out what our options are.  Can we try again?  ;)

> It should always point to the origin/parent window.

Origin of which?  Right now it basically points to the origin of the script that ended up with an uncaught exception.  Not the script that _threw_ it, but the script that ended up being first on the stack and didn't catch it.  In this case that's the Firebug script.

> In any case, is there any other solid way how to find out what is the window/page where
> the error is coming from?

There never has been.  Consider two web pages, X and Y.  A function F in X, triggered off a setTimeout in X, calls a function G in Y.  G throws.  The outer window for the exception was, and remains, X, since that's where the exception is last uncaught.

> Firebug is re-throwing the exception. That's the problem.

That doesn't matter, really.  The original exception has the chrome windowid in this case.

Thinking about this more, the Firebug behavior of setting onreadystatechange is pretty bizarre, because that's a page-visible operation.  In particular, basic checks on whether onreadystatechange has the value the page actually set would fail if run in the page.  Which brings us back to my original question: Do you _really_ need to set onreadystatechange, or do you just need to get readystatechange events?
(In reply to Boris Zbarsky (:bz) from comment #11)
> Thinking about this more, the Firebug behavior of setting onreadystatechange
> is pretty bizarre
Agree, there have been several problems with this approach in the past.

> None of that answered my question.  Which is unfortunate, because answering
> it might let us figure out what our options are.  Can we try again?  ;)
Sorry, sure we can try again :)

> Or would just a readystatechange listener be enough?
I like the idea of using readystatechange listener to observe XHR life-time. I just don't know if it's enough. It works in most cases except if the XHR object calls abort() inside the 'readystatechange' callback.

onreadystatechange is used mostly to catch when the XHR finishes and get the response.

> That is, are you trying to _filter_ readystatechange events the page gets,
> or just be notified of changes?
be notified of changes.

Honza
OK, great!  So what happens if instead of setting onreadystatechange you do:

  xhr.addEventListener("readystatechange", myfunction, true);

?  That should certainly make sure you get called before onreadystatechange.  If the page itself uses capturing listeners that abort() you would still have a problem, but you already have that problem.
Sounds promising, but... I tried that and I've got confusing results:

1) If I add the listener (useCapture == true) on the web-page, the listener is executed before the onreadystatechange callback. Note, that |useCapture == false| makes no difference (listener is still executed before the callback)

2) If I add the listener (useCapture == true) in Firebug (when http-on-modify-request is sent) the listener is executed after the callback.

Here is how Firebug is getting the xhr object from nsIHttpChannel, |request| (passed as the subject to http-on-modify-request):

Briefly:

var xhr = request.notificationCallbacks.getInterface(Ci.nsIXMLHttpRequest);

xhr.addEventListener("readystatechange", function(event) { }, true);

Am I doing something wrong?

Honza
If I understand this correctly, FB wants to have a listener which is called before anything else?
Bug 779306 added support for catch-all listener, which is in the event listener list
before anything else.
Would that work for FB?
(In reply to Olli Pettay [:smaug] from comment #15)
> If I understand this correctly, FB wants to have a listener which is called
> before anything else?
Yes

> Bug 779306 added support for catch-all listener, which is in the event
> listener list
> before anything else.
> Would that work for FB?
Sounds promising, I'll check it out.

Honza
So, I re-implementd Firebug's XHR spy using nsIEventListenerService. addListenerForAllEvents, and all seem to be OK (the listener is properly executed before the default handler).

Thanks Boris and Olli I appreciate your help!

Honza
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.