670896 - Add inner window ID and timestamp to nsIScriptError2

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

As discussed with dcamp, I am opening a bug about the possibility of adding a timestamp and an inner window ID property to all of the nsIScriptError2 messages.

For background please see bug 611032 comment 58 and 60.

We need to display cached errors that belong to a specific window based on the inner ID, so we don't show too many messages from too many pages that share the same outer window ID. This needs to happen when the Web Console is open.

We want to use the existing messages cache (the nsIConsoleService), but nsIScriptError2 messages do not have timestamps, and we cannot order them in the output.

The lack of inner window IDs would force us to do something like getInnerWindowID(getOuterWindowById(error.outerWindowID)) for each message, then compare it to inner ID of the window where the Web Console is open, which is quite ugly. Even more so, we'd have to compare the inner ID with each inner ID of all of the iframes/frames in the current page.

It would be more efficient to have the innerWindowID property already available on the error object and just filter the messages accordingly.

Thoughts? Is it worth adding an innerWindowID to nsIScriptError2 messages or we should just do the manual checks in the Web Console?

Thank you!

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  Can you look up why we used the outer window ID to start with there?  I recall inner making more sense to me too, but us going with outer for some reason because there was some problem with inner window IDs there.

Comment 2

[Mihai Sucan [:msucan]]

AFAIK, we used outer window IDs because they are more stable in terms of associating the Web Console to a specific tab.

We map Web Consoles to outer IDs of window.top.

We could probably mix the two. Looking through the code we might be able to use only a window inner ID from the nsIScriptError2, so if the outer ID is removed perhaps it's not *that* bad.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, ok.  It's probably not a problem to report both the inner and the outer ID.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: wip

This is my first try at adding an inner ID and a timestamp to the nsIScriptError2 messages.

I am not sure if the nsGlobalWindow.h #include is allowed in this part of the code. Also, I wasn't sure if I should use nsRefPtrs/nsCOMPtr.

Comments are very much welcome! Thank you!

Comment 5

[Boris Zbarsky [:bzbarsky]]

Nothing really guarantees that that's the right inner, right?  The inner reporting the error might not in fact be the current one (an edge case, but it can happen).

Comment 6

[Mihai Sucan [:msucan]]

(In reply to comment #5)
> Nothing really guarantees that that's the right inner, right?  The inner
> reporting the error might not in fact be the current one (an edge case, but
> it can happen).

Well, if the outer window ID is correct, then the inner window we get for the window object we find should be correct. Can the inner window change while initWithWindowId() is called? Or?

If yes, the inner window can change so fast, then sure, it's a very-very close to the edge case. I doubt we should cater to such cases. Am I wrong?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Can the inner window change while initWithWindowId() is called?

No, but multiple inner windows can exist all for the same outer window at the same time.  That's why the getter there is asking for the _current_ inner window.

Comment 8

[Boris Zbarsky [:bzbarsky]]

And to be clear, I would think that non-current inner windows can report errors, in general.

Comment 9

[Mihai Sucan [:msucan]]

(In reply to comment #8)
> And to be clear, I would think that non-current inner windows can report
> errors, in general.

Do scripts execute in non-current inner windows?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Usually not.  Or at least we try to make them not do that.  We don't always succeed.

And some of our warnings can come from script in a current window doing something to the DOM of a non-current window...

Comment 11

[Mihai Sucan [:msucan]]

(In reply to comment #10)
> Usually not.  Or at least we try to make them not do that.  We don't always
> succeed.
> 
> And some of our warnings can come from script in a current window doing
> something to the DOM of a non-current window...

Ouch.

Please let me know if you want me to tackle this problem, and how I should do so. The only option I have in mind, for this, would be to find again all those places where nsIScriptError2 messages are created, and add an innerWindowID to initWithWindowID(). I would avoid this, since it's not-so-nice. Nonetheless, if I have to do so, no problem. :)

Thanks!

Comment 12

[Boris Zbarsky [:bzbarsky]]

I think that would be the right solution, yes...

We could just pass in the inner window id at those callsites and derive the outer window id from it as you do, I think.  Going in that direction doesn't have any problems.

The good thing is that just a grep for initWithWindowID will find the callsites....

Comment 13

[Mihai Sucan [:msucan]]

Attachment: part 1 - nsIScriptError2 and util methods

Comment 14

[Mihai Sucan [:msucan]]

Attachment: part 2 - layout/style changes

Comment 15

[Mihai Sucan [:msucan]]

Attachment: part 3 - libpr0n changes

Comment 16

[Mihai Sucan [:msucan]]

Attachment: part 4 - web workers changes

Comment 17

[Mihai Sucan [:msucan]]

Attachment: part 5 - xpconnect changes

Comment 18

[Mihai Sucan [:msucan]]

Attachment: part 6 - nsEventSource, nsWebSocket, nsJSEnvironemnt and others

Last patch.

Please note that these apply cleanly on top of the fx-team repo.

Also ... I don't write C++ code often. ;) Please let me know of any changes I have to make.

Thank you!

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 547189 [details] [diff] [review]
part 1 - nsIScriptError2 and util methods

nsJSUtils::GetCurrentlyRunningCodeWindowID should probably grow an "Outer" somewhere in there.

Why not put the window ids next to each other in the idl?

Did you really mean the timestamp to claim to be wrt some platform-dependent 0 offset when it's really a PRTime (which has a quite well-defined 0)?  In any case, the value you're storing there is in microseconds, not in milliseconds.  And it's signed; storing it in an unsigned member is sort of broken.  What are you actually trying to do with this timestamp?

The rest of this looks fine, except for the whole not compiling bit (in fact, I assume you will qfold all these changes before landing, so we won't have known-busted changesets in the tree), but the timestamp thing needs to be sorted out.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 547198 [details] [diff] [review]
part 5 - xpconnect changes

r=me modulo the timestamp stuff.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 547194 [details] [diff] [review]
part 3 - libpr0n changes

The existing WindowID() getter and mWindowId members and the SetWindowID setter should probably grow an "Outer" somewhere.  r=me with that.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 547196 [details] [diff] [review]
part 4 - web workers changes

Again, the existing variables should grow an "outer".  r=me with that.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 547200 [details] [diff] [review]
part 6 - nsEventSource, nsWebSocket, nsJSEnvironemnt and others

And again, add "outer" to existing names as needed.

>+        nsCOMPtr<nsPIDOMWindow> innerWin = win->GetCurrentInnerWindow();

This can be a weak ref.  Actually, the same in nsJSUtils now that I think back to it.  And in the expat driver.

I'm starting to feel like a window id struct that has both IDs would make sense perhaps....  Not really fodder for this bug.  But just curious: was there a reason to not just pass in the inner window id from all the callsites and look up the outer window id in the init code?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 547192 [details] [diff] [review]
part 2 - layout/style changes

s/mWindowID/mOuterWindowID/

>+  nsCOMPtr<nsPIDOMWindow> window;

This should be an nsPIDOMWindow*, right?

>+  if (window && mParent) {

Should be !window.

r=me with that

Comment 25

[Mihai Sucan [:msucan]]

(In reply to comment #19)
> Comment on attachment 547189 [details] [diff] [review] [review]
> part 1 - nsIScriptError2 and util methods
> 
> nsJSUtils::GetCurrentlyRunningCodeWindowID should probably grow an "Outer"
> somewhere in there.
> 
> Why not put the window ids next to each other in the idl?
> 
> Did you really mean the timestamp to claim to be wrt some platform-dependent
> 0 offset when it's really a PRTime (which has a quite well-defined 0)?  In
> any case, the value you're storing there is in microseconds, not in
> milliseconds.  And it's signed; storing it in an unsigned member is sort of
> broken.  What are you actually trying to do with this timestamp?

The code in the patch comes from what I was able to find with mxr. I forgot to check the definition of PR_Now(). Having checked it now, I see it's a PRTime.

What we want is a way to sort by timestamp nsIScriptError2 messages in the Web Console. The timestamp resolution is not critical in this case.

Would switching to the PRTime type be sufficient to address the problem?

(no experience with timers, timestamps in Gecko, sorry about that)

(In reply to comment #21)
> Comment on attachment 547194 [details] [diff] [review] [review]
> part 3 - libpr0n changes
> 
> The existing WindowID() getter and mWindowId members and the SetWindowID
> setter should probably grow an "Outer" somewhere.  r=me with that.

Will update the patch accordingly.

(In reply to comment #23)
> Comment on attachment 547200 [details] [diff] [review] [review]
> part 6 - nsEventSource, nsWebSocket, nsJSEnvironemnt and others
> 
> And again, add "outer" to existing names as needed.
> 
> >+        nsCOMPtr<nsPIDOMWindow> innerWin = win->GetCurrentInnerWindow();
> 
> This can be a weak ref.  Actually, the same in nsJSUtils now that I think
> back to it.  And in the expat driver.
> 
> I'm starting to feel like a window id struct that has both IDs would make
> sense perhaps....  Not really fodder for this bug.  But just curious: was
> there a reason to not just pass in the inner window id from all the
> callsites and look up the outer window id in the init code?

This is what I understood from the initial discussion above that would be fine. Nonetheless, I can make the change to only send inner ids to InitWithWindowID(), and get the outer window id inside the InitWithWindowID() method. Should be simple now that I have all the callsites grepped.

This would also eliminate the need for a struct - which I thought of when I saw we always have the outer and inner ids coupled, in so many places.

Shall I make the switch to inner ID only? Is it fine to have a call similar to what I had ... in InitWithWindowId() that finds the outer window id based on the inner id?


Thanks for your reviews and comments!

Comment 26

[Boris Zbarsky [:bzbarsky]]

> What we want is a way to sort by timestamp

Do you want to guarantee non-decreasing timestamps?  Because PR_Now() makes no such guarantee.... Sadly, there's no clean way to expose mozilla::TimeStamp to JS right now.

How about you make the IDL say |long long|, document it as an opaque timestamp, and use PR_Now() for now?  Then you can change the underlying implementation later if you decide the fact that PR_Now() can go backwards is a problem.

> I can make the change to only send inner ids to InitWithWindowID()

On second thought, there might be cases when we have an outer window but no inner window... Do we care?  If not, then please do make this change, since it simplifies the caller-side code.

> Is it fine to have a call similar to what I had ... in InitWithWindowId()
> that finds the outer window id based on the inner id?

I think so, yes.

Comment 27

[Mihai Sucan [:msucan]]

(In reply to comment #26)
> > What we want is a way to sort by timestamp
> 
> Do you want to guarantee non-decreasing timestamps?  Because PR_Now() makes
> no such guarantee.... Sadly, there's no clean way to expose
> mozilla::TimeStamp to JS right now.
> 
> How about you make the IDL say |long long|, document it as an opaque
> timestamp, and use PR_Now() for now?  Then you can change the underlying
> implementation later if you decide the fact that PR_Now() can go backwards
> is a problem.

Interesting point. Hopefully it's not going to be a problem.

> > I can make the change to only send inner ids to InitWithWindowID()
> 
> On second thought, there might be cases when we have an outer window but no
> inner window... Do we care?  If not, then please do make this change, since
> it simplifies the caller-side code.

When would it happen to have an inner window without an outer window? Is this an often case? If we have no outer ID we'll fail to display the message while the Web Console is open. I'd rather prefer the other case - when we fail to cache a message.


> > Is it fine to have a call similar to what I had ... in InitWithWindowId()
> > that finds the outer window id based on the inner id?
> 
> I think so, yes.

Great, thanks!

Comment 28

[Boris Zbarsky [:bzbarsky]]

> When would it happen to have an inner window without an outer window?

This can't happen.

But you can have an outer window without an inner window (e.g. a newly created window with nothing in it yet, or a window that's already been closed).  So the situation that might happen is that you _could_ get an outer id, but not an inner one; if we do everything based on the inner id we won't get the outer id in that case.  But I suspect this should be very rare, if it happens at all.

Comment 29

[Mihai Sucan [:msucan]]

(In reply to comment #28)
> > When would it happen to have an inner window without an outer window?
> 
> This can't happen.
> 
> But you can have an outer window without an inner window (e.g. a newly
> created window with nothing in it yet, or a window that's already been
> closed).  So the situation that might happen is that you _could_ get an
> outer id, but not an inner one; if we do everything based on the inner id we
> won't get the outer id in that case.  But I suspect this should be very
> rare, if it happens at all.

Ah, then it's not something to really worry about. Thanks! Will update the patch accordingly.

Comment 30

[Mihai Sucan [:msucan]]

Attachment: wip 2

Updated the patch as requested. Folded all changes into a single patch.

Switched from outer window IDs to inner window IDs, and added logic to determine the outer window ID from the inner ID.

It's all fine, but we do have a regression. The HUDService tests fail for the DOM:HTML category and for Web Sockets.

It seems that nsIDocument->InnerWindowID() returns 0, and we get no outer window ID for such cases. Any reason why inner window ID would be 0?

For two failures, it would be a shame to have to go back to code that sends both inner and outer IDs. Any ideas how to fix these cases?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> It seems that nsIDocument->InnerWindowID() returns 0, and we get no outer
> window ID for such cases. Any reason why inner window ID would be 0?

Is there an inner window in that case?

Comment 32

[Mihai Sucan [:msucan]]

(In reply to comment #31)
> > It seems that nsIDocument->InnerWindowID() returns 0, and we get no outer
> > window ID for such cases. Any reason why inner window ID would be 0?
> 
> Is there an inner window in that case?

Did some more debugging and found the problem. The nsJSUtils::GetCurrentlyRunningCodeInnerWindowID() method did not always work as intended - the window object I get from the script context was not always an outer window (as I assumed). So now I fixed to check if I get an inner window or an outer window and things work fine.

Will attach the updated patch. Thank you!

Comment 33

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Updated the patch. Changes:

- fix the nsJSUtils::GetCurrentlyRunningCodeInnerWindowID() to work as intended.
- rebase. Ben Turner's DOM Web Workers patch has landed, and I had to change the code to work with the new stuff there.

Please let me know if further changes are needed. Thank you!

Comment 34

[Boris Zbarsky [:bzbarsky]]

Huh.  Getting an inner window from the script context is really weird!  Blake, why would that happen?

Comment 35

[Mihai Sucan [:msucan]]

Attachment: switch timestamp from micro to milliseconds

As asked in bug 611032 comment 63 I have switched from microseconds to milliseconds, such that we match the timer resolution of Date.now() from JavaScript. I hope this is fine.


Looking forward to your review. We would like to land this patch in time for Firefox 8. Thank you!

Comment 36

[Boris Zbarsky [:bzbarsky]]

Oh, nevermind.  I see what's going on.  You're not getting anything from the script context.  You're getting a window from the JSObject, which should totally be the inner window.

Do you ever even hit the !IsInnerWindow() codepath in GetCurrentlyRunningCodeInnerWindowID?

Also, is GetCurrentlyRunningCodeOuterWindowID still used after this patch?  Or can it be removed?

Comment 37

[Boris Zbarsky [:bzbarsky]]

And yes, I can try to get this reviewed today/tomorrow.

Comment 38

[Mihai Sucan [:msucan]]

(In reply to comment #36)
> Oh, nevermind.  I see what's going on.  You're not getting anything from the
> script context.  You're getting a window from the JSObject, which should
> totally be the inner window.

That was most-likely a wording mistake on my side. I am not very used with the exact JSAPI parlance. I need to be more exact.

> Do you ever even hit the !IsInnerWindow() codepath in
> GetCurrentlyRunningCodeInnerWindowID?

I haven't checked, but I will.


> Also, is GetCurrentlyRunningCodeOuterWindowID still used after this patch? 
> Or can it be removed?

It's no longer used. Shall I remove it?

Looking forward to your review. Thank you!

Comment 39

[Boris Zbarsky [:bzbarsky]]

> I haven't checked, but I will.

Please do.  I'm pretty sure that codepath is not reachable.

> It's no longer used. Shall I remove it?

Yes, please.

Comment 40

[Mihai Sucan [:msucan]]

Attachment: remove GetCurrentlyRunningCodeOuterWindowID()

Updated the patch. All HUDService tests show that IsInnerWindow() always returns true, but I cannot be 100% sure. I asked in #jsapi and luke suggested that JS_GetGlobalForScopeChain() returns an inner window, and he also pointed me to mxr:

http://mxr.mozilla.org/mozilla-central/source/js/src/jscntxtinlines.h#56

As such, I removed GetCurrentlyRunningCodeOuterWindowID() and I also made the code always assume that we get an inner window.

Looking forward to your review. Thanks!

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 550712 [details] [diff] [review]
remove GetCurrentlyRunningCodeOuterWindowID()

r=me.  Thanks for your patience here!

Comment 42

[Mihai Sucan [:msucan]]

(In reply to Boris Zbarsky (:bz) from comment #41)
> Comment on attachment 550712 [details] [diff] [review] [diff] [details] [review]
> remove GetCurrentlyRunningCodeOuterWindowID()
> 
> r=me.  Thanks for your patience here!

Thank you for your review and guidelines! It was my pleasure. Every time I dig into Gecko I learn new stuff.

Comment 43

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch

Rebased the patch on top of the fxteam repo.

Comment 44

[Mihai Sucan [:msucan]]

Comment on attachment 555701 [details] [diff] [review]
[checked-in] rebased patch

Landed:
http://hg.mozilla.org/integration/fx-team/rev/8e1f1cb42303

Comment 45

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

Comment on attachment 555701 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/8e1f1cb42303

Comment 46

[Nickolay_Ponomarev]

dev-doc-needed: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIScriptError2

Comment 47

[Eric Shepherd [:sheppy]]

Docs updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIScriptError2

Also updated Firefox 9 for developers.