Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: rc, Assigned: bz)

Tracking

({dev-doc-complete})

Trunk
mozilla2.0b7
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
adding some interested CCs.
(Assignee)

Updated

7 years ago
Assignee: general → nobody
Component: JavaScript Engine → DOM
Priority: -- → P1
QA Contact: general → general

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 603706
(Assignee)

Comment 3

7 years ago
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).
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Updated

7 years ago
Assignee: nobody → bzbarsky
(Assignee)

Comment 4

7 years ago
Created 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.
(Assignee)

Comment 5

7 years ago
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.
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?
(Assignee)

Comment 7

7 years ago
> 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.
(Assignee)

Comment 8

7 years ago
That said, I can see how mapping from window id to <browser> might be a pain.  How is Fennec solving that problem right now?
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.
(Assignee)

Comment 10

7 years ago
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.
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...).
(Assignee)

Comment 12

7 years ago
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?
(Reporter)

Comment 13

7 years ago
that's right. The console has a single consoleservice listener.
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...)
(Assignee)

Comment 15

7 years ago
> 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

7 years ago
(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.
(Assignee)

Comment 17

7 years ago
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?
(Reporter)

Comment 18

7 years ago
Either of these two methods would be preferable to the onError hack.
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.
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.
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!
(Assignee)

Comment 22

7 years ago
> 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.
(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.
(Reporter)

Comment 24

7 years ago
absolutely. Sounds like an easy solution.
(Assignee)

Updated

7 years ago
Attachment #484408 - Attachment is obsolete: true
(Assignee)

Comment 25

7 years ago
Created attachment 484854 [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.
(Assignee)

Comment 26

7 years ago
Created attachment 484855 [details] [diff] [review]
part 2.  Add a way to get an outer window object given its window id via windowutils.
(Assignee)

Updated

7 years ago
Attachment #484854 - Attachment is obsolete: true
(Assignee)

Comment 27

7 years ago
Created 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.
(Assignee)

Comment 28

7 years ago
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....
Attachment #484856 - Flags: review?(mrbkap)
(Assignee)

Updated

7 years ago
Attachment #484855 - Attachment is obsolete: true
(Assignee)

Comment 29

7 years ago
Created attachment 484857 [details] [diff] [review]
part 2.  Add a way to get an outer window object given its window id via windowutils.
(Assignee)

Comment 30

7 years ago
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?
Attachment #484857 - Flags: review?(mrbkap)

Updated

7 years ago
Attachment #484857 - Flags: review?(mrbkap) → review+

Updated

7 years ago
Attachment #484856 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 31

7 years ago
Beltzner said this is blocking b7 if I get it in expeditiously.
blocking2.0: --- → beta7+
Whiteboard: [need landing[
(Assignee)

Updated

7 years ago
Whiteboard: [need landing[ → [need landing]
(Assignee)

Comment 32

7 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b7
Depends on: 606070

Comment 33

7 years ago
Holy Crap! Yay!
(Reporter)

Comment 34

7 years ago
fantastic turn-around. Thanks bz and mrbkap!

Updated

7 years ago
Keywords: dev-doc-needed
Documented:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIScriptError2
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMWindowUtils#getOuterWindowWithId()
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.