605492 - Mechanism for tying a page's external script errors and warnings to a particular web-page or window object

Status: RESOLVED FIXED

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

Description

[Rob Campbell [:rc] (:robcee)]

in bug 605241, we're trying to figure out where errors are coming from in external scripts loaded in a webpage. We have a window.onerror() handler currently, but this might be easier to obtain from the JS Engine itself.

from Mihai's comment 0 in that bug:
Currently we use the window.onerror event handler to work around the problem
with the nsIConsoleService observer which fails to catch errors from external
scripts.

However, the window.onerror event handler fails to catch warnings. We need to
fix this.

Comment 1

[Rob Campbell [:rc] (:robcee)]

adding some interested CCs.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Uh, no.  This isn't a duplicate.  The other bug has way wider scope.

I plan to fix this bug for all the things that went through window.onerror.  I don't plan to fix the other at the same time (though one could build on what I do in this bug to fix the other).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Add an nsIScriptError2 interface that exposes an outer window id, and set the window id for script errors coming through the DOM JS error reporter.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 484408 [details] [diff] [review]
Add an nsIScriptError2 interface that exposes an outer window id, and set the window id for script errors coming through the DOM JS error reporter.

This is somewhat suboptimal in the sense that I'd prefer we put an inner window id on the error, but I'm not seeing a quick way to get one in the JSEnvironment code right this second, and this is good enough for non-edge cases (errors triggered during document navigation) for the web console, I think.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

The web console, as currently written, really needs a window object reference (from which it can find the containing <browser> via its docshell's chromeEventHandler, and from there find the actual console object). That's kind of a pain to get given a window ID, right? Can we make that easier somehow?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> really needs a window object reference

Then it's kinda broken, no?  I'm totally opposed to adding new APIs the expose window object references, because they completely fail in the e10s case.

Comment 8

[Boris Zbarsky [:bzbarsky]]

That said, I can see how mapping from window id to <browser> might be a pain.  How is Fennec solving that problem right now?

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For the specific problem of console logging, we remote that from the content-->chrome process, and the content process has a window so all is well.

More generally, I don't know what window IDs are so I can't speak for that.  CC'ing mfinkle who should be able to.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Window ids are unique 64-bit numbers assigned to every window object.  The idea was that they can be used to represent window identity without having to hand out actual window objects (both for e10s, and so people wouldn't hold leaky refs to them, and so that we can represent the identity of _destroyed_ window objects).

I suppose I could just stick a window (or "context", as nsISupports) on nsIScriptError2 if that would be better all around.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I wasn't that we need to give exception objects window references, but rather pointing out that the web console kind of needs a way to get them somehow, at least for the moment. A getWindowById() method or somesuch would do, but I don't know of any existing code that tracks all windows in that manner, and I could see that being troublesome. The patch in bug 587734 adds a getWindowByWindowId in the frontend that uses the window enumerator, but that is problematic in several ways - slow, tabbrowser-specific, doesn't find subdocument windows, etc.

The Web Console could alternatively do something like keeps its own list of child window IDs for a given <browser> (not sure how that would be populated...) and use that for the mapping, I guess, but keeping that list up to date across navigations/document creation and such seems like it would be ugly and error-prone.

I haven't given much thought to how this should work in an e10s world (or even how it should ideally work now - I'm mostly looking into this code after the fact...).

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.

It sounds like the web console has one single consoleservice listener, not listener-per-tab?  If it had the latter it could just walk its tab's windows (using window.frames) looking for the id in question.  I suppose that could also get slow in rare cases....

Another option is to have the error include both the ID of the originating window _and_ of its window.top.  That should make it pretty easy for console to tell what's going on, I hope.  Does make logging the error slower, though...

Jonas, thoughts?

Comment 13

[Rob Campbell [:rc] (:robcee)]

that's right. The console has a single consoleservice listener.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

It has one nsIConsoleService listener, yeah. Those can't be per-tab. It also has window.onerror handlers attached to every window (except subframes, until bug 598438 is fixed anyways), but those associate their errors differently (via a closure to the actual console object until bug 587734 lands, after which it will start using IDs and we'll have the same problem...)

Comment 15

[Boris Zbarsky [:bzbarsky]]

> It also has window.onerror handlers attached to every window

Yes, that's what this bug is trying to make unnecessary.

Rob, what API would make things simplest/sanest for you here?  Would exposing the id of .top on the error (which you could then map to a tab using a hashtable, basically) work?

Comment 16

[David Dahl :ddahl]

(In reply to comment #15)

> Rob, what API would make things simplest/sanest for you here?  Would exposing
> the id of .top on the error (which you could then map to a tab using a
> hashtable, basically) work?

That would be great - much better than the solution we use now. The downside is, as gavin mentioned, our method to map back to a window is slow and Firefox-specific.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Yes, I'm saying you should use a different method.  ;)

I suppose the other option is to just stick the window on there for now.  Jonas, thoughts?

Comment 18

[Rob Campbell [:rc] (:robcee)]

Either of these two methods would be preferable to the onError hack.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Using the windowID would be the safest in the long run. When Fx goes e10s, storing the window won't be usable, unless handles auto-magically save the day.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think a window reference is what we should do for now. It's hard to say what things will look like in an e10s world so lets worry about that post FF4.

I'd even say stick a .window and a .context property on there. That way consumers can rely on .window always being there and being a window, and .context can contain the relevant element or XHR or whatever as appropriate per error.

Comment 21

[Mihai Sucan [:msucan]]

I would "vote" for the originating window ID, and that only. The window.top window object can be inferred from the originating window ID, and we can find the window objects based on ID.

The getWindowById() method we have is suboptimal, but far better than the rest of the work arounds we have now.

Boris: can't we have a Gecko level getWindowById() method? One that does a better job in finding the exact window object for the given ID.

At the HUDService level we are obviously left with the only choice but to hold a map of window IDs and hudIds (HeadsUpDisplay objects). Based on messages coming from each window we would then easily and more nicely display messages in each Web Console, as needed.

With regards to a .context property: that wouldn't be bad at all. It would be nice if it would point to the relevant element or XHR. But then, we get into the land of: what is "relevant"? The XHR object or the <script> element that includes the code ... which triggered the message? And so on.

Ultimately, I believe we should focus: focus on what we *need*. That is a window ID and ideally a getWindowById().

We can improve our implementation of getWindowById(): we can hold a map of window IDs to hudIds, and a map of huIds to HUD objects. Each HUD object already points to the contentWindow (which is the top window from each xul:browser of each tab).

When we want to display an error message coming from the nsIConsoleService we don't really need the window object reference at all. We want to find our HUD object, actually. To do that we just need the window ID.

Thanks!

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Boris: can't we have a Gecko level getWindowById() method? 

You sure can, as long as you're ok with it returning null if the window is dead.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #22)
> You sure can, as long as you're ok with it returning null if the window is
> dead.

That'd be fine. Just adding windowID to nsIScriptErrors + adding a getWindowById() method in the core sounds like a good plan to me.

Comment 24

[Rob Campbell [:rc] (:robcee)]

absolutely. Sounds like an easy solution.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add an nsIScriptError2 interface that exposes an outer window id, and set the window id for script errors coming through the DOM JS error reporter.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a way to get an outer window object given its window id via windowutils.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add an nsIScriptError2 interface that exposes an outer window id, and set the window id for script errors coming through the DOM JS error reporter.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 484856 [details] [diff] [review]
part 1.  Add an nsIScriptError2 interface that exposes an outer window id, and set the window id for script errors coming through the DOM JS error reporter.

I went with "ID" over "Id" because the existing windowutils functions capitalize it that way....

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a way to get an outer window object given its window id via windowutils.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 484857 [details] [diff] [review]
part 2.  Add a way to get an outer window object given its window id via windowutils.

And here I went with "Id" to be consistent with GetElementById.... let me know if you think that's a terrible idea?

Comment 31

[Boris Zbarsky [:bzbarsky]]

Beltzner said this is blocking b7 if I get it in expeditiously.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Pushed on m-c:

http://hg.mozilla.org/mozilla-central/rev/ec4198d99816
http://hg.mozilla.org/mozilla-central/rev/768ada595b2b
http://hg.mozilla.org/mozilla-central/rev/a72ca32bac1d (to make the new method not usable from untrusted script)

and on b7 relbranch:

http://hg.mozilla.org/mozilla-central/rev/666a6214dc7d
http://hg.mozilla.org/mozilla-central/rev/d2ddc5f80339
http://hg.mozilla.org/mozilla-central/rev/455860e4a4ad

Comment 33

[David Dahl :ddahl]

Holy Crap! Yay!

Comment 34

[Rob Campbell [:rc] (:robcee)]

fantastic turn-around. Thanks bz and mrbkap!

Comment 35

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIScriptError2
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils#getOuterWindowWithId()