606498 - Make sure the new nsIScriptError2 is used in all possible places

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Mihai Sucan [:msucan]]

In bug 605492 we have introduced the new nsIScriptError2 interface which allows us to attach window IDs to script errors. The patch includes the changes necessary for the nsJSEnvironment code to use the new API, however we have more places where we can use the new API.

We need to make sure that wherever is possible the new nsIScriptError2 interface is used, such that the Web Console gets the window ID.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch. This patch changes Gecko code to use the new nsIScriptError2 API which includes the originating window ID.

Added window IDs to the following categories:

- Canvas. nsCanvasRenderingContext2D
- DOM Events. nsDOMEvents, nsHTMLDocument, nsGlobalWindow and nsHtml5StreamParser. Generally, this code generates warnings when deprecated API is used.
- HTML. nsFormSubmission. Warnings related to incorrect <form> markup.
- DOM3 Load. nsXMLDocument. Again warnings about deprecated API.
- DOM:HTML. nsDOMClassInfo. Prints warnings about the use of document.all and other messages.
- DOM Window. nsGlobalWindow.
- DOM. nsDocument and nsXULDocument. Warning about empty getElementById().
- DOM Worker javascript. nsDOMThreadService and nsDOMWorkerPool.
- ImageMap. nsImageMap. Warnings about broken imagemap coords.
- CSS Loader. SheetLoadData. Stylesheet loader warnings.
- CSS Parser. nsCSSParser and nsCSSScanner. All CSS parse failures.
- FrameConstructor. nsCSSFrameConstructor::ProcessChildren().
- SVG. nsSVGUtils. Some SVG-related warnings.
- malformed-xml. nsExpatDriver. XML parser errors.
- component javascript. mozJSComponentLoader.
- XPConnect JavaScript. nsXPCComponents_Utils::ReportError(), XPCConvert::JSErrorToXPCException() and nsXPCWrappedJSClass::CheckForException().
- XBL, XBL Content Sink, xbl javascript, XBL Prototype Handler.

This patch fixes bugs 603711, 603714, 603723, 603727, 603730 and 607088.

I have included a few mochitests for the new cases handled by the Web Console.

Sorry for the huge patch. Please let me know if you want it split.

Comment 2

[Mihai Sucan [:msucan]]

Asking for the blocking2.0+ flag. This patch does much needed work at Gecko level for unearthing various warnings and errors triggered by web pages, making them show up in the Web Console.

For an exhaustive list of the types of messages that will be displayed after applying this patch, please see the patch description comment #1.

Thanks!

Comment 3

[Mihai Sucan [:msucan]]

The patch I attached depends on the work Patrick did in bug 597136 - this is where the Web Console code is updated to use the new nsIScriptError2 window ID. Without that patch, the new HUDService mochitests fail.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486643 [details] [diff] [review]
proposed patch

OK, I stopped reading partway through, but here are the things I'd like to see fixed before reading this again:

1)  Having a class-static method that takes a class instance as argument and is
    always passed |this| from members is silly.  Just make it a member method. 
    This is in nsDocument.
2)  Instead of copy/pasting a bajillion copies of the "get the window id from
    the document" code, just have an overload of ReportToConsole that takes an
    nsIDocument as an argument, gets the window id from the document, and calls
    the window id overload.
3)  Instead of copy/pasting multiple copies of the "init an error that may not
    be an nsIScriptError2" code, just have a utility method for it. 
    Alternately, I'd be fine with assuming that the NS_SCRIPTERROR_CONTRACTID
    contract implies implementation of nsIScriptError2 (so just change the
    thing the return value of do_CreateInstance is assigned to to
    nsCOMPtr<nsIScriptError2>, for all the cases that always use it if
    implemented).  
4)  I'd really appreciate not having to review megapatches.  Please split the
    change to nsContentUtils::ReportToConsole into a separate diff?

Comment 7

[Mihai Sucan [:msucan]]

(In reply to comment #4)
> Comment on attachment 486643 [details] [diff] [review]
> proposed patch
> 
> OK, I stopped reading partway through, but here are the things I'd like to see
> fixed before reading this again:

Thanks for your quick review!


> 1)  Having a class-static method that takes a class instance as argument and is
>     always passed |this| from members is silly.  Just make it a member method. 
>     This is in nsDocument.

Oops, indeed. Will update the patch.


> 2)  Instead of copy/pasting a bajillion copies of the "get the window id from
>     the document" code, just have an overload of ReportToConsole that takes an
>     nsIDocument as an argument, gets the window id from the document, and calls
>     the window id overload.

Yeah, good point. Will attach the updated patch.


> 3)  Instead of copy/pasting multiple copies of the "init an error that may not
>     be an nsIScriptError2" code, just have a utility method for it. 
>     Alternately, I'd be fine with assuming that the NS_SCRIPTERROR_CONTRACTID
>     contract implies implementation of nsIScriptError2 (so just change the
>     thing the return value of do_CreateInstance is assigned to to
>     nsCOMPtr<nsIScriptError2>, for all the cases that always use it if
>     implemented).  

Agreed. Still, nsIConsoleService->LogMessage() doesn't take nsIScriptError2 objects (they don't seem to inherit from nsIConsoleMessage?). So, I have to do QI when I send it to the service. Will attach the updated patch. Please let me know if the changes I did are fine. If I have to make further changes, I don't mind.

> 4)  I'd really appreciate not having to review megapatches.  Please split the
>     change to nsContentUtils::ReportToConsole into a separate diff?

I wasn't sure how you prefer patches. I have split the patch into three pieces: "nsIScriptError to nsIScriptError2", "ReportToConsole changes" and "HUDService changes and tests".

Thanks again!

Comment 8

[Mihai Sucan [:msucan]]

Attachment: Part 1: switch to nsIScriptError2

Comment 9

[Mihai Sucan [:msucan]]

Attachment: Part 2: changes for ReportToConsole

Comment 10

[Mihai Sucan [:msucan]]

Attachment: Part 3: HUDService tests

Comment 16

[Boris Zbarsky [:bzbarsky]]

> So, I have to do QI when I send it to the service

Yeah, that's fine (sorry for lag; wasn't cced).

> I wasn't sure how you prefer patches.

I can't think of anyone who prefers large patches...  Time needed to review, for me at least, is quadratic or worse in patch length, unless the patch is purely mechanical (then it's more or less linear).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486944 [details] [diff] [review]
Part 1: switch to nsIScriptError2

I'll have to audit the dynamic script context stuff carefully here... might take a few days.

For the CSS parser, I would really prefer not adding more code than we have to into the initialization codepath; that's actually a hot codepath, and css errors occur rarely compared to how often that codepath is run.  Better to lazily init mWindowID, imo.  Why are we getting things off the child loader, by the way?  Is that just a hack to deal with us not always having a sheet?

The changes in this patch are all basically independent from each other, right?

Comment 18

[Mihai Sucan [:msucan]]

(In reply to comment #17)
> Comment on attachment 486944 [details] [diff] [review]
> Part 1: switch to nsIScriptError2
> 
> I'll have to audit the dynamic script context stuff carefully here... might
> take a few days.

No problem. Thanks for looking into the code! I hope I haven't made silly errors, hehe.

> For the CSS parser, I would really prefer not adding more code than we have to
> into the initialization codepath; that's actually a hot codepath, and css
> errors occur rarely compared to how often that codepath is run.  Better to
> lazily init mWindowID, imo.  Why are we getting things off the child loader, by
> the way?  Is that just a hack to deal with us not always having a sheet?

Oh, I didn't know that CSSParserImpl::SetChildLoader() and CSSParserImpl::SetStyleSheet() are hot code paths. I did this because in the nsCSSScanner I was more "limited": no access to the sheet, and no access to the loader either.

Would it be better to pass the loader and the sheet to the scanner entirely? If I do that, then I can lazily read mWindowID when OutputError is called.

To answer your question wrt. getting things off the child loader: yes. I noticed the sheet object is not always available in the parser. This happens even for external stylesheets. Ultimately, I was "forced" to read the window ID through the child loader *as well*.

If you believe I should do things otherwise, please let me know.


> The changes in this patch are all basically independent from each other, right?

In general, yes, but as you saw ... nsCSSScanner and nsCSSParser changes are related to each other, and similarly there are changes for DOM Workers code in two files. Otherwise, changes are indeed independent from each other. Hopefully this means they are easier to review.

Thank you again!

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486944 [details] [diff] [review]
Part 1: switch to nsIScriptError2

>+++ b/content/xbl/src/nsXBLDocumentInfo.cpp
>@@ -51,16 +51,18 @@
> XBL_ProtoErrorReporter(JSContext *cx,
>+    nsIScriptGlobalObject *globalObject =
>+      nsJSUtils::GetDynamicScriptGlobal(cx);
>+    if (globalObject) {
>+      nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(globalObject);
>+      if (win)

Have you tested this?  Do you actually ever get a non-null window here?  I'd expect globalObject here to always be an nsXBLDocGlobalObject.

So all this part of the patch does is give you a false sense of security, as far as I can tell.

>+++ b/content/xml/document/src/nsXMLDocument.cpp

>+      PRUint64 windowID = 0;
>+      nsCOMPtr<nsPIDOMWindow> win = callingDoc ?
>+        callingDoc->GetWindow() : this->GetWindow();
>+
>+      if (win) {
>+        windowID = win->WindowID();
>+      }

I would prefer to have a shared method somewhere that takes an nsIDocument (or heck, lives on nsIDocument as an inline method; that's not an API change in terms of binary compat!) and returns the window id or 0 if there is no window.

>+            (nsCOMPtr<nsIScriptError>)(do_QueryInterface(errorObject)));

Separate line for the nsCOMPtr, please.  This throughout the patch.

I sorta wish I'd made nsIScriptError2 inherit from nsIScriptError... :(  We should get a followup bug filed to make this saner post-2.0.

>+++ b/dom/base/nsDOMClassInfo.cpp
>+  nsIScriptGlobalObject *globalObject = nsJSUtils::GetDynamicScriptGlobal(cx);

Do you really want the dynamic script global?  I'd think you want the static one...  Have you tested this with multiple script globals involved?  Think two frames, with script in frame A being called, which calls a function in frame B, which does whatever triggets this code.  The dynamic script global in that situation is frame A; the static one is frame B.  I'd think you want the latter here; the's certainly where your filename and line number are coming from.

NOte that GetStaticScriptGlobal will likely return an inner window; you will need to outerize.

>+++ b/dom/src/threads/nsDOMThreadService.cpp
>+  PRUint64 windowID = worker->Pool()->WindowID();

Why do you need this temporary up here?  Why not just do |worker->Pool()->WindowID()| in the call to InitWithWindowID?

>+++ b/dom/src/threads/nsDOMWorkerPool.cpp
>+  nsCOMPtr<nsPIDOMWindow> win = aDocument->GetWindow();

Yeah, we definitely need that helper.  ;)

>+++ b/dom/src/threads/nsDOMWorkerPool.h
>+  PRUint64 WindowID() {

Should be a const method.

>@@ -100,11 +104,13 @@ private:
>+  PRUint64 mWindowID;

And if it'll compile, this should be a const member (with the helper, you can use the initializer to set it, which I think is allowed for const members).

>+++ b/js/src/xpconnect/loader/mozJSComponentLoader.cpp
>+        nsIScriptGlobalObject *globalObject =
>+            nsJSUtils::GetDynamicScriptGlobal(cx);

Did you test this?  I fully expect this to return null in all cases, since the JS component has its own private JSContext it uses for everything.  So again, this is just giving misleading warm and fuzzy feelings, I think.

>+++ b/js/src/xpconnect/src/xpccomponents.cpp
>@@ -2911,38 +2915,49 @@ nsXPCComponents_Utils::ReportError()

>+    nsIScriptGlobalObject *globalObject = nsJSUtils::GetDynamicScriptGlobal(cx);

Should this be a static script global?

It might be good to have a helper for getting a window id from an nsIScriptGlobalObject too, instead of copy/pasting the code.  And if it can take null, then we can just pass in the results of Get*ScriptGlobal as needed.

>+++ b/js/src/xpconnect/src/xpcconvert.cpp
>+            nsJSUtils::GetDynamicScriptGlobal(cx);

Static script global?

>                                 static_cast<nsIScriptError*>(data),

Need data.get() here.

>+++ b/js/src/xpconnect/src/xpcwrappedjsclass.cpp
>+                                nsIScriptGlobalObject *globalObject =
>+                                    nsJSUtils::GetDynamicScriptGlobal(cx);

Static script global?

>+++ b/layout/style/Loader.h

I'd prefer we spin the CSS stuff out into a separate patch, check with dbaron whether he's ok with the nsCSSScanner storing a ref to the CSSParserImpl, and if so do that so the scanner can lazily get the data it wants.

Note that we have CSS parsers that take neither a sheet nor a loader; we may want to fix that...

>+++ b/parser/htmlparser/src/nsExpatDriver.cpp
>+    nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow();

That doesn't need to be a strong ref; same elsewhere.

>+      nsCOMPtr<nsIScriptGlobalObject> global =

Neither does that.

>+      if (win && !win->IsOuterWindow()) {
>+        win = win->GetOuterWindow();

win->GetOuterWindow() is ok even if |win| is already an outer window, no?

You should probably init mWindowID to 0 in the ctor.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486945 [details] [diff] [review]
Part 2: changes for ReportToConsole

>+++ b/content/base/public/nsContentUtils.h
>+   *   @param [aWindowId=0] (Optional) Tells the message origin by the ID of the
>+              outer window object.

How about:

  The window ID of the outer window the message originates from.

?

>+++ b/content/base/src/nsContentUtils.cpp
>+      (nsCOMPtr<nsIScriptError>)(do_QueryInterface(errorObject)));

Separate line for the nsCOMPtr, please.

>+    nsPIDOMWindow* win = aDocument->GetWindow();

This should use that helper from the previous patch.

>+++ b/content/base/src/nsDocument.cpp
>+                                  static_cast<nsIDocument*>(this));

You don't need the explicit cast.  Likewise elsewhere in this file.

>+++ b/content/canvas/src/nsCanvasRenderingContext2D.cpp

>+    nsCOMPtr<nsIDocument> doc;
>+    if (mCanvasElement) {
>+        nsCOMPtr<nsIContent> content = do_QueryInterface(
>+            static_cast<nsIDOMHTMLCanvasElement*>(mCanvasElement));
>+        doc = content->GetOwnerDoc();
>+    }

nsIDocument* doc;
if (mCanvasElement) {
  doc = HTMLCanvasElement()->GetOwnerDoc();
}

>+++ b/content/html/document/src/nsHTMLDocument.cpp
>+                                  static_cast<nsIDocument*>(aDoc));

Don;t need the cast.  Same elsewhere in this file.

>+++ b/content/xbl/src/nsXBLPrototypeHandler.cpp
>+  nsCOMPtr<nsIDocument> doc;

This can be a weak ref; just make sure to init it to null.

>@@ -454,17 +454,17 @@ nsXBLStreamListener::Load(nsIDOMEvent* a
>+                                      "XBL", bindingDocument);

Note that bindingDocument will never give you a window id.  Not sure you can do better here, though.

There seem to be a few places in the XBL code where you could pass a null URI now that you pass a document.

>+++ b/content/xul/document/src/nsXULDocument.cpp
>                                     mDocumentURI,

No need for that, right?

>+                                    static_cast<nsIDocument*>(this));

Or for the cast.

>+++ b/dom/base/nsGlobalWindow.cpp
>+      nsCOMPtr<nsIDocument> doc = do_QueryInterface(this->GetExtantDocument());

Just use mDoc.

The rest looks good.  This is much closer to being done than part 1.... ;)

Please post both updated patches and interdiffs when you have changes, by the way?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 486946 [details] [diff] [review]
Part 3: HUDService tests

I'm sorta assuming these test the right things, etc.... though given my comments above they're clearly not testing all the things you touched.

Comment 22

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 23

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 24

[Mihai Sucan [:msucan]]

(In reply to comment #19)
> Comment on attachment 486944 [details] [diff] [review]
> Part 1: switch to nsIScriptError2
> 
> >+++ b/content/xbl/src/nsXBLDocumentInfo.cpp
> >@@ -51,16 +51,18 @@
> > XBL_ProtoErrorReporter(JSContext *cx,
> >+    nsIScriptGlobalObject *globalObject =
> >+      nsJSUtils::GetDynamicScriptGlobal(cx);
> >+    if (globalObject) {
> >+      nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(globalObject);
> >+      if (win)
> 
> Have you tested this?  Do you actually ever get a non-null window here?  I'd
> expect globalObject here to always be an nsXBLDocGlobalObject.
> 
> So all this part of the patch does is give you a false sense of security, as
> far as I can tell.

You make good points here and for the mozJSComponentLoader changes. I went through MXR when I started working on these changes, and I did not know all these intricacies you've pointed out - eg. the mozJSComponentLoader has its own private JSContext. Because of these, and because these error messages are not currently relevant for the Web Console, I'll simply drop the changes to these two files (nsXBLDocumentInfo and mozJSComponentLoader) in the upcoming updated patches.


> >+++ b/content/xml/document/src/nsXMLDocument.cpp
> 
> >+      PRUint64 windowID = 0;
> >+      nsCOMPtr<nsPIDOMWindow> win = callingDoc ?
> >+        callingDoc->GetWindow() : this->GetWindow();
> >+
> >+      if (win) {
> >+        windowID = win->WindowID();
> >+      }
> 
> I would prefer to have a shared method somewhere that takes an nsIDocument (or
> heck, lives on nsIDocument as an inline method; that's not an API change in
> terms of binary compat!) and returns the window id or 0 if there is no window.

Great! I have added an inline helper method. Will be included in the updated patches.


> >+            (nsCOMPtr<nsIScriptError>)(do_QueryInterface(errorObject)));
> 
> Separate line for the nsCOMPtr, please.  This throughout the patch.

Will do.

> I sorta wish I'd made nsIScriptError2 inherit from nsIScriptError... :(  We
> should get a followup bug filed to make this saner post-2.0.

Yeah, I noticed this problem and I was like "ouch".


> >+++ b/dom/base/nsDOMClassInfo.cpp
> >+  nsIScriptGlobalObject *globalObject = nsJSUtils::GetDynamicScriptGlobal(cx);
> 
> Do you really want the dynamic script global?  I'd think you want the static
> one...  Have you tested this with multiple script globals involved?  Think two
> frames, with script in frame A being called, which calls a function in frame B,
> which does whatever triggets this code.  The dynamic script global in that
> situation is frame A; the static one is frame B.  I'd think you want the latter
> here; the's certainly where your filename and line number are coming from.

Again, this is part of intricacies I did not know about. Thanks for pointing it out. I switched the code to use the static script global.


> >+++ b/dom/src/threads/nsDOMThreadService.cpp
> >+  PRUint64 windowID = worker->Pool()->WindowID();
> 
> Why do you need this temporary up here?  Why not just do
> |worker->Pool()->WindowID()| in the call to InitWithWindowID?

Will update accordingly.

> >+++ b/dom/src/threads/nsDOMWorkerPool.cpp
> >+  nsCOMPtr<nsPIDOMWindow> win = aDocument->GetWindow();
> 
> Yeah, we definitely need that helper.  ;)

Yep. :)

> >+++ b/dom/src/threads/nsDOMWorkerPool.h
> >+  PRUint64 WindowID() {
> 
> Should be a const method.
> 
> >@@ -100,11 +104,13 @@ private:
> >+  PRUint64 mWindowID;
> 
> And if it'll compile, this should be a const member (with the helper, you can
> use the initializer to set it, which I think is allowed for const members).

Made them both consts. It works. Thanks!


> >+++ b/js/src/xpconnect/src/xpccomponents.cpp
> >@@ -2911,38 +2915,49 @@ nsXPCComponents_Utils::ReportError()
> 
> >+    nsIScriptGlobalObject *globalObject = nsJSUtils::GetDynamicScriptGlobal(cx);
> 
> Should this be a static script global?
> 
> It might be good to have a helper for getting a window id from an
> nsIScriptGlobalObject too, instead of copy/pasting the code.  And if it can
> take null, then we can just pass in the results of Get*ScriptGlobal as needed.

Added a helper to nsJSUtils to find the outer window ID by a given JSContext. It's more useful, I think. In this patch it always starts with a JSContext, then an nsIScriptGlobalObject, then the nsPIDOMWindow, and finally the outer window ID.

I'll attach the updated patch, and you let me know if it's fine.


> >+++ b/js/src/xpconnect/src/xpcconvert.cpp
> >+            nsJSUtils::GetDynamicScriptGlobal(cx);
> 
> Static script global?

Will update.

> >                                 static_cast<nsIScriptError*>(data),
> 
> Need data.get() here.

Will update.

> >+++ b/js/src/xpconnect/src/xpcwrappedjsclass.cpp
> >+                                nsIScriptGlobalObject *globalObject =
> >+                                    nsJSUtils::GetDynamicScriptGlobal(cx);
> 
> Static script global?

Will update.

> >+++ b/layout/style/Loader.h
> 
> I'd prefer we spin the CSS stuff out into a separate patch, check with dbaron
> whether he's ok with the nsCSSScanner storing a ref to the CSSParserImpl, and
> if so do that so the scanner can lazily get the data it wants.

As you saw, current I use the two SetChildLoader() and SetStyleSheet() method of the CSSParserImpl class to determine the window ID, coming either from the Loader or from the nsCSSStyleSheet (their owning documents). I pass the window ID to the nsCSSScanner::Init() method through CSSParserImpl::InitScanner(). All this, such that nsCSSScanner::OutputError() can use the window ID.

You think I should pass the CSSParserImpl object to nsCSSScanner and determine the window ID lazily in OutputError()? (and cache it, so we don't do it for *every* error)


> Note that we have CSS parsers that take neither a sheet nor a loader; we may
> want to fix that...

Uh oh. When does this happen? It seems to work with the common cases I tested with. Also, what can I do about this then? Isn't it beyond the purpose of this patch?


> >+++ b/parser/htmlparser/src/nsExpatDriver.cpp
> >+    nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow();
> 
> That doesn't need to be a strong ref; same elsewhere.
> 
> >+      nsCOMPtr<nsIScriptGlobalObject> global =
> 
> Neither does that.
> 
> >+      if (win && !win->IsOuterWindow()) {
> >+        win = win->GetOuterWindow();
> 
> win->GetOuterWindow() is ok even if |win| is already an outer window, no?
> 
> You should probably init mWindowID to 0 in the ctor.

Thanks for your review! Once I update part 2 as well, I'll attach both of the patches.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> You think I should pass the CSSParserImpl object to nsCSSScanner and determine
> the window ID lazily in OutputError()?

Yes.

> When does this happen?

Most obviously, if you set .text on the .media of a stylesheet.  That should probably pass in the media list's stylesheet instead.

Comment 26

[Kevin Dangoor]

mass change: filter mail on BLOCKERSETTING

Comment 27

[Mihai Sucan [:msucan]]

(In reply to comment #20)
> Comment on attachment 486945 [details] [diff] [review]
> Part 2: changes for ReportToConsole
> 
> >@@ -454,17 +454,17 @@ nsXBLStreamListener::Load(nsIDOMEvent* a
> >+                                      "XBL", bindingDocument);
> 
> Note that bindingDocument will never give you a window id.  Not sure you can do
> better here, though.

What can I do then? Shall I remove the code changes here? Or shall I leave it as it is?


> Please post both updated patches and interdiffs when you have changes, by the
> way?

Will do.

Thanks for your review!

Comment 28

[Mihai Sucan [:msucan]]

Attachment: Part 1: switch to nsIScriptError2

Updated the patch with changes based on your review. Thanks!

Comment 29

[Mihai Sucan [:msucan]]

Attachment: Part 1: interdiff

Interdiff for part 1, showing differences between the old patch and the new one.

(Please note that CSS Parser and Loader changes have been moved out into a separate patch, which means I'll attach an interdiff for the new CSS changes patch as well.)

Comment 30

[Mihai Sucan [:msucan]]

Attachment: Part 2: changes for ReportToConsole

Updated patch, based on your review comments. Thanks!

Comment 31

[Mihai Sucan [:msucan]]

Attachment: Part 2: interdiff

Interdiff for part 2, comparing the changes between the old patch and the new one. Again, please note that the CSS stuff have been moved into a separate patch, as requested.

Comment 32

[Mihai Sucan [:msucan]]

Attachment: Part 3: CSS-related changes

The new patch for CSS-related changes.

For dbaron: This patch adds a window ID to the CSS Parser error messages. This is split out from the previous patches, and it includes changes requested by Boris.

Please note that my initial proposal was to record the window ID in the CSSParserImpl code, whenever SetChildLoader() and SetStyleSheet() were invoked. This seemed to work fine for me, but Boris pointed out those are hot paths and that I'd better move the code into the scanner OutputError() method, to lazily find the window ID.

This new patch works in some tests I did, but the mochitest from the upcoming patch (part 4) fails. I assume that lazily retrieving the window ID in OutputError() causes it to not catch some loaders/sheets. Is it possible that by the time OutputError() is executed, for the child loader and/or mSheet to no longer be available?

Looking forward to your review and comments on how to improve this patch. Thanks!

Comment 33

[Mihai Sucan [:msucan]]

Attachment: Part 3: interdiff

Interdiff for part 3 which shows the differences between the old patch and the new one.

Please note that "old patch" means: the CSS related stuff taken from the old parts 1 and 2 combined into one.

Comment 34

[Mihai Sucan [:msucan]]

Attachment: Part 4: HUDService tests

Rebased the patch. No other changes. Thanks for the r+!

The CSS Parser test fails, see comment #32.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> Shall I remove the code changes here? 

I think so (esp. since web script can't load XBL anyway, right?).

Comment 36

[Mihai Sucan [:msucan]]

Attachment: Part 2: changes for ReportToConsole

Updated patch: removed the chunk in question (nsXBLStreamListener::Load()).

Comment 37

[Mihai Sucan [:msucan]]

Attachment: Part 2: interdiff

Updated interdiff.

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 492963 [details] [diff] [review]
Part 3: CSS-related changes

nsCSSParser.cpp:

+  nsCSSStyleSheet* GetStyleSheet() const {
+    return mSheet ? mSheet.get() : nsnull;
+  }

Just "return mSheet;" would be sufficient (except I'm suggesting
removing the code anyway).


You shouldn't be passing an nsCSSParser to the scanner.  nsCSSParser is
just a wrapper class that shouldn't be passed around like that.  

Instead, you should make nsCSSScanner::Init take the style sheet and
CSS loader as parameters.  (You should keep the bit that does lazy
computation in OutputError based on them, though.)

This means you also don't need to add the methods to nsCSSParser or
CSSParserImpl or the mOwningParser to CSSParserImpl.


I think this is fine with those changes, but I'd like to see the revised
patch, so marking review-.

Also, when you post patches, please include the author and commit message in the patch header.

Comment 39

[Mihai Sucan [:msucan]]

Attachment: Part 3: CSS-related changes

Thanks David for your review and comments. I have updated the patch as requested.

I also added a new FindOwningWindowID() method to the nsCSSStyleSheet class to allow me to find the window ID for more cases, not just based on the owning document - this also checks for the owning node, owning rule and for the parent sheet. I hope this change is fine.

Finally, I looked into all of the Gecko codebase to find places where the nsCSSParser is used. I found that with this patch we almost cover all of the cases we might be interested of (for the Web Console). Still, there are some I'd like to ask you about:

http://dxr.mozilla.org/mozilla-central/content/canvas/src/nsCanvasRenderingContext2D.cpp.html#2256

That's an example of the nsCanvasRenderingContext2D usage of the nsCSSParser. We get no reference to the document->CSSLoader(), nor do we have a sheet reference (obviously). For such cases, we won't get the window ID, and we'll loose any CSS parsing errors coming from the canvas 2D context code.

Can I pass a reference to the CSSLoader()? Is that safe? When the parser is constructed. It seems like a fairly trivial change. ;)

Also, nsHTMLFragmentContentSink makes use of the CSS parser, without any reference to the CSS Loader / sheet:

http://dxr.mozilla.org/mozilla-central/content/html/document/src/nsHTMLFragmentContentSink.cpp.html#1037
http://dxr.mozilla.org/mozilla-central/content/html/document/src/nsHTMLFragmentContentSink.cpp.html#1153

Both seem fixable by pointing to the CSSLoader() of mTargetDocument, when the parser is constructed. Would this be fine?

The attached patch works fine with style attributes in HTML pages, <style> fragments, and <link rel=stylesheet>, and with errors coming from @imported sheets.

Please let me know if further changes are needed. Thanks again!

(will attach an interdiff, for easier reviewing)

Comment 40

[Mihai Sucan [:msucan]]

Attachment: Part 3: interdiff

Interdiff showing the differences from the previous patch and the latest one (for part 3).

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #39)
> I also added a new FindOwningWindowID() method to the nsCSSStyleSheet class to
> allow me to find the window ID for more cases, not just based on the owning
> document - this also checks for the owning node, owning rule and for the parent
> sheet. I hope this change is fine.

Do you know if all of the pieces of this function actually help some cases?

> Finally, I looked into all of the Gecko codebase to find places where the
> nsCSSParser is used. I found that with this patch we almost cover all of the
> cases we might be interested of (for the Web Console). Still, there are some
> I'd like to ask you about:
> 
> http://dxr.mozilla.org/mozilla-central/content/canvas/src/nsCanvasRenderingContext2D.cpp.html#2256
> 
> That's an example of the nsCanvasRenderingContext2D usage of the nsCSSParser.
> We get no reference to the document->CSSLoader(), nor do we have a sheet
> reference (obviously). For such cases, we won't get the window ID, and we'll
> loose any CSS parsing errors coming from the canvas 2D context code.
> 
> Can I pass a reference to the CSSLoader()? Is that safe? When the parser is
> constructed. It seems like a fairly trivial change. ;)

That seems fine, although I'd leave a comment that the reason to pass the loader is for error reporting, since it's a little odd to use a CSS loader as the error reporting object.

> Also, nsHTMLFragmentContentSink makes use of the CSS parser, without any
> reference to the CSS Loader / sheet:
> 
> http://dxr.mozilla.org/mozilla-central/content/html/document/src/nsHTMLFragmentContentSink.cpp.html#1037
> http://dxr.mozilla.org/mozilla-central/content/html/document/src/nsHTMLFragmentContentSink.cpp.html#1153
> 
> Both seem fixable by pointing to the CSSLoader() of mTargetDocument, when the
> parser is constructed. Would this be fine?

Sounds fine, but again with a comment explaining why.  (Also, the second one does pass a sheet.)

Comment 42

[Mihai Sucan [:msucan]]

(In reply to comment #41)
> (In reply to comment #39)
> > I also added a new FindOwningWindowID() method to the nsCSSStyleSheet class to
> > allow me to find the window ID for more cases, not just based on the owning
> > document - this also checks for the owning node, owning rule and for the parent
> > sheet. I hope this change is fine.
> 
> Do you know if all of the pieces of this function actually help some cases?

I haven't performed exhaustive tests to point at a specific case. However, given the nature of the sheets, they are in a tree-like relation (imported sheets, within imported sheets, and so on). Also, because the owning document can be null, parent can be null, etc, I believe it makes sense to check whichever is available to find the window ID.

If you believe I should remove the function, please let me know, so I can do this in the next patch update. (with the changes below)

> > Can I pass a reference to the CSSLoader()? Is that safe? When the parser is
> > constructed. It seems like a fairly trivial change. ;)
> 
> That seems fine, although I'd leave a comment that the reason to pass the
> loader is for error reporting, since it's a little odd to use a CSS loader as
> the error reporting object.

Oky, thanks! Yeah, it's odd, but the only other solution would be to change the signature of ParseStyleAttribute (and friends) to allow me to pass the document object or directly the window ID, or some other similar change. So I think it's nicer at the moment to just pass the CSS loader, and include a comment why.

Is the patch fine, otherwise?

Comment 43

[Mihai Sucan [:msucan]]

Attachment: Part 3: CSS-related changes

Updated the patch to include refs to the CSS Loader object as discussed above.

Comment 44

[Mihai Sucan [:msucan]]

Attachment: Part 3: interdiff

Interdiff showing the latest patch changes.

Comment 45

[Mihai Sucan [:msucan]]

Attachment: Part 4: HUDService tests

Included a test for CSS parser errors coming from Canvas code.

Comment 46

[Mihai Sucan [:msucan]]

Attachment: Part 4: interdiff

Interdiff showing only the changes in the latest patch.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 492953 [details] [diff] [review]
Part 1: switch to nsIScriptError2

>+++ b/content/base/public/nsIDocument.h
>+  PRUint64 OuterWindowID()

This should be a const method, right?  No need to change what the return value is; just make this |PRUint64 OuterWindowID const()|.

>+++ b/dom/base/nsJSUtils.h
>+  static PRUint64 OuterWindowIDByContext(JSContext *aContext, JSBool aUseDynamicScript = JS_FALSE);

No one is using that second arg, nor do I expect anyone to.  Just remove it and the corresponding codepath, please?

I think this would be better named GetCurrentlyRunningCodeWindowID() or something.  That should make it clear what it's doing.

You probably want to get sr from jst on this bit.

r=me with the above nits.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 493052 [details] [diff] [review]
Part 2: changes for ReportToConsole

Fwiw, the interdiff for part 2 clearly doesn't match the actual part 2 patch.  :(

>+++ b/content/xbl/src/nsXBLPrototypeHandler.cpp
>+  nsIDocument* doc = nsnull;
>+  if (mPrototypeBinding) {
>+    nsXBLDocumentInfo* docInfo = mPrototypeBinding->XBLDocumentInfo();
>+    if (docInfo) {
>+      doc = docInfo->GetDocument().get();

No, that's bad.  You just leaked the document.  Given that docInfo->GetDocument() returns an already_AddRefed, which I didn't realize when I read your first version here, you do need to make |doc| an nsCOMPtr like in your original patch...  Sorry about that.  :(  But next time, please don't just do the cast-till-it-compiles thing, ok?

>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -5958,21 +5958,20 @@ nsGlobalWindow::Close()
>+          "DOM Window", mDoc);  // Better name for the category?

For what it's worth, this will report the window id and URI of the window that failed to close, not of the code that was trying to do the closing.  May not matter much, I guess.

r=me with that leak fixed.

Comment 49

[Mihai Sucan [:msucan]]

(In reply to comment #47)
> Comment on attachment 492953 [details] [diff] [review]
> Part 1: switch to nsIScriptError2
> 
> >+++ b/dom/base/nsJSUtils.h
> >+  static PRUint64 OuterWindowIDByContext(JSContext *aContext, JSBool aUseDynamicScript = JS_FALSE);
> 
> No one is using that second arg, nor do I expect anyone to.  Just remove it and
> the corresponding codepath, please?

Sure, I can do that.


> I think this would be better named GetCurrentlyRunningCodeWindowID() or
> something.  That should make it clear what it's doing.

I named it like that, because it takes a JSContext argument, and because it's not always necessarily the "currently running code". It just happens to be used, most often, in such cases. I considered the method just finds the outer window ID from the given JSContext.

> You probably want to get sr from jst on this bit.

Thanks for the review+! Will ask for sr from jst once I update the patch.

Comment 50

[Mihai Sucan [:msucan]]

(In reply to comment #48)
> Comment on attachment 493052 [details] [diff] [review]
> Part 2: changes for ReportToConsole
> 
> Fwiw, the interdiff for part 2 clearly doesn't match the actual part 2 patch. 
> :(

Afaik it does.

> >+++ b/content/xbl/src/nsXBLPrototypeHandler.cpp
> >+  nsIDocument* doc = nsnull;
> >+  if (mPrototypeBinding) {
> >+    nsXBLDocumentInfo* docInfo = mPrototypeBinding->XBLDocumentInfo();
> >+    if (docInfo) {
> >+      doc = docInfo->GetDocument().get();
> 
> No, that's bad.  You just leaked the document.  Given that
> docInfo->GetDocument() returns an already_AddRefed, which I didn't realize when
> I read your first version here, you do need to make |doc| an nsCOMPtr like in
> your original patch...  Sorry about that.  :(  But next time, please don't just
> do the cast-till-it-compiles thing, ok?

No worries.


> >+++ b/dom/base/nsGlobalWindow.cpp
> >@@ -5958,21 +5958,20 @@ nsGlobalWindow::Close()
> >+          "DOM Window", mDoc);  // Better name for the category?
> 
> For what it's worth, this will report the window id and URI of the window that
> failed to close, not of the code that was trying to do the closing.  May not
> matter much, I guess.

That's fine, we just need to show the warning in the Web Console.

Thanks for your review+! Will attach the updated patch.

Comment 51

[Mihai Sucan [:msucan]]

(In reply to comment #47)
> Comment on attachment 492953 [details] [diff] [review]
> Part 1: switch to nsIScriptError2
> 
> >+++ b/content/base/public/nsIDocument.h
> >+  PRUint64 OuterWindowID()
> 
> This should be a const method, right?  No need to change what the return value
> is; just make this |PRUint64 OuterWindowID const()|.

I didn't make the OuterWindowID() a const method because the method it calls is not a const method either, and compilation fails:

../../dist/include/nsIDocument.h: In member function ‘PRUint64 nsIDocument::OuterWindowID() const’:
../../dist/include/nsIDocument.h:690: error: passing ‘const nsIDocument’ as ‘this’ argument of ‘nsPIDOMWindow* nsIDocument::GetWindow()’ discards qualifiers

Comment 52

[Mihai Sucan [:msucan]]

Attachment: Part 1: switch to nsIScriptError2

Updated the patch.

Asking for superreview from jst for the nsJSUtils change.

Comment 53

[Mihai Sucan [:msucan]]

Attachment: Part 2: changes for ReportToConsole

Updated the patch.

Comment 54

[Boris Zbarsky [:bzbarsky]]

> I considered the method just finds the outer window ID from the given JSContext.

The point is that there are several different ways of doing it; the method name should try to make clear which one it will use.

> Afaik it does.

No, it really doesn't.  For example, the interdiff is missing the differences in nsHTMLDocument.cpp (and a bunch of other files, as far as I can tell).

> That's fine, we just need to show the warning in the Web Console.

Yes, but you'll show it in the wrong web console in some cases (for the wrong window).

> because the method it calls is not a const method either

Bah.  It should be!  Please file a followup bug on this?

Comment 55

[Boris Zbarsky [:bzbarsky]]

And I was serious about the function naming in nsJSUtils.  I'm not ok with OuterWindowIDByContext.

Comment 56

[Mihai Sucan [:msucan]]

Boris: I don't mind. I can change the method name, but I am not sure what name to use. Are you fine with GetCurrentlyRunningCodeWindowID()?

Also, with regards to window.close() I can change it to use nsIJSContextStack to get the current JSContext, to find the window ID, script file name, line number, etc. Would this be fine?

Thanks!

Comment 57

[Boris Zbarsky [:bzbarsky]]

> Are you fine with GetCurrentlyRunningCodeWindowID()?

Well, I _did_ suggest it.  ;)

> Also, with regards to window.close() I can change it to use nsIJSContextStack

Let's not worry about this right now.  Maybe a followup to decide what we want there?

Comment 58

[Mihai Sucan [:msucan]]

Attachment: Part 1: switch to nsIScriptError2

Renamed the nsJSUtils::OuterWindowIDByContext() method to nsJSUtils::GetCurrentlyRunningCodeWindowID().

Also, reported the follow-up bugs 618662 and 618664, as requested by Boris. Not sure if I correctly picked the components, hehe. :)

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 497132 [details] [diff] [review]
Part 1: switch to nsIScriptError2

Blake, this is the patch I was talking about.  Could you double-check whether the nsJSUtils stuff needs to enter compartments?

Comment 60

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 497132 [details] [diff] [review]
Part 1: switch to nsIScriptError2

Because we maintain the invariant that (this is pseudocode) cx->scopeChain()->compartment() == cx->compartment, there is no need to enter a compartment when operating on the current scope chain's global object. So this looks good to me.

Comment 61

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 496893 [details] [diff] [review]
Part 3: CSS-related changes

+/* virtual */ PRUint64
+nsCSSStyleSheet::FindOwningWindowID() const
+{
+  PRUint64 windowID = 0;
+  if (mDocument) {
+    windowID = mDocument->OuterWindowID();
+  }
+
+  if (windowID == 0 && mOwningNode) {
+    nsRefPtr<nsIContent> node = do_QueryObject(mOwningNode);

nsIContent is an interface (sort of), so this should be
  nsCOMPtr<nsIContent> node = do_QueryInterface(mOwningNode);


+    if (node) {
+      nsIDocument* doc = node->GetOwnerDoc();
+      if (doc) {
+        windowID = doc->OuterWindowID();
+      }
+    }
+  }
+
+  if (windowID == 0 && mOwnerRule) {
+    nsRefPtr<nsICSSRule> rule = do_QueryObject(mOwnerRule);

This should be an nsCOMPtr like above, except that no QI is needed
at all, you can just start with mOwnerRule->GetStyleSheet() since
nsICSSImportRule inherits from nsICSSRule.

+    if (rule) {
+      nsRefPtr<nsIStyleSheet> sheet = rule->GetStyleSheet();

nsCOMPtr.

+      if (sheet) {
+        nsRefPtr<nsCSSStyleSheet> cssSheet = do_QueryObject(sheet);

This nsRefPtr/do_QueryObject is correct.



r=dbaron with that plus the comments requested in comment 41.

Comment 62

[Mihai Sucan [:msucan]]

(In reply to comment #61)
> Comment on attachment 496893 [details] [diff] [review]
> Part 3: CSS-related changes
> 
> r=dbaron with that plus the comments requested in comment 41.

The patch already includes the comments for why we pass the CSS Loader to the CSS Parser at initialization. Or do you refer to something else? (that's what comment 41 asks, if I correctly understood it)

I will update the patch with the rest of the changes you requested.

Thank you for the review+!

Comment 63

[Mihai Sucan [:msucan]]

Attachment: Part 3: CSS-related changes

Updated the patch as requested by dbaron.

And with this update, we are done with all the patches. Thanks to everyone who reviewed them!

Comment 64

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

http://hg.mozilla.org/mozilla-central/rev/b9aa4b7efd92
http://hg.mozilla.org/mozilla-central/rev/01bbeb378ff3
http://hg.mozilla.org/mozilla-central/rev/1c10afc9c6cc
http://hg.mozilla.org/mozilla-central/rev/0b88e1ab00c1