229437 - "WARNING: Failed to get a global Object Owner" suppresses actual JS error message

Status: RESOLVED FIXED

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

Description

[Karsten Düsterloh]

Whenever ScriptErrorReporter fails to get a global object owner for a particular
 JS error message, it dumps 
"WARNING: Failed to get a global Object Owner, file
f:/Projekte/Mozilla/Mozilla.org/mozilla/dom/src/base/nsJSEnvironment.cpp, line 156"
to the console and exits, not stating which actual message it should have
displayed. The global object owner problem is not relevant for dumping to the
console, so we should at least do that.

Comment 1

[Karsten Düsterloh]

Attachment: Simple patch

Instead of just returning when having no global object owner, only dump to the
console

Comment 2

[Karsten Düsterloh]

Attachment: Same patch but with -w

same patch without white space for better reviewing ;-)

Comment 3

[Boris Zbarsky [:bzbarsky]]

The problem is that nsXULPrototypeDocument::NewXULPDGlobalObject uses a shared
global object with no owner for all chrome documents...

Comment 4

[Karsten Düsterloh]

@timeless, regarding your comments on IRC:
| <timeless> jump to the part near
| <timeless> errorObject(do_CreateInstance("@mozilla.org/scripterror;1"));
[...]
| <timeless> i'm thinking i want that error in the jsconsole no matter what

The problem Boris describes in comment #3 would not be solved "here" in
nsJSEnvironment.cpp, would it? Then it would still be good to handle missing
owners (and hence missing docShells) gracefully and dump to the console even if
we can't dump to the jsconsole? That's exactly what my patch does...

And I don't know how to fix the problem Boris describes anyway. :|

Comment 5

[timeless]

There are two consoles, one is the jsconsole (tools>web dev...>javascript
console), the other is where stdout/stderr live. Your patch only lets things go
to the latter.

see
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsFormSubmission.cpp#1121

Comment 6

[Brendan Eich [:brendan]]

I'm gonna move this to DOM, in the hope that jst or peterv can help make the
error reporter work even if it can't get an owner from the global object.  The
shared XULPDGlobalObject is a footprint/perf win we'd like to keep, and I don't
see why NS_ScriptErrorReporter can't cope.  Someone pls. correct me if I'm
missing some hard case here.

/be

Comment 7

[Peter Van der Beken [:peterv]]

Brendan: the default owner for dom bugs is [e-mail],
[e-mail] is an account I use for watching it. I can rename it
if you want to avoid confusion.

Comment 8

[Karsten Düsterloh]

Attachment: Handle missing owner gracefully

Handle a missing owner gracefully and dump the error message now both to
jsconsole and console.

Comment 9

[Karsten Düsterloh]

Attachment: Same patch as 138018, but with -w

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 138019 [details] [diff] [review]
Same patch as 138018, but with -w

@@ -174,8 +168,14 @@ NS_ScriptErrorReporter(JSContext *cx,
	 msg.AssignWithConversion(message);
       }

-      //send error event first, then proceed
       nsCOMPtr<nsIDocShell> docShell;
+      nsCOMPtr<nsIScriptGlobalObjectOwner> owner;
+      if(NS_FAILED(globalObject->GetGlobalObjectOwner(getter_AddRefs(owner)))
||
+	  !owner) {
+	 NS_WARNING("Failed to get a global Object Owner");
+      }
+      else {
+	 //send error event first, then proceed
       globalObject->GetDocShell(getter_AddRefs(docShell));
       if (docShell && !JSREPORT_IS_WARNING(report->flags)) {

No need to warn here, IMO. This is now obviously not an error case, so we
should just handle it w/o spitting out warnings etc. So remove the above check,
just call GetGlobalObjectOwner() (no need to check for errors on that call
either), and just make the check that follows this code execute even if there's
no owner (unless that doesn't work for some reason).

Change that, and I'll have one more look.

Comment 11

[Karsten Düsterloh]

Attachment: Drop warning since we report the real error anyway

If I drop the owner checks, calling GetDocShell will scream with

WARNING: waaah!, file
f:/Projekte/Mozilla/Mozilla.org/mozilla/content/xul/document/src/nsXULPrototypeDocument.cpp,
line 858

if bugs like bug 229450 happen before we actually have a window. (Very helpful
warning, indeed. :( )

The docShell will stay null in this case, so we might just as well test the
owner here and avoid calling GetDocShell entirely.
(All implementations of GetDocShell I could find return NS_OK, but that isn't
something I'd rely upon, so I used NS_SUCCEEDED to be safe.)

Comment 12

[Karsten Düsterloh]

Attachment: Same as 138303, but with -w

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 138303 [details] [diff] [review]
Drop warning since we report the real error anyway

+      nsCOMPtr<nsIScriptGlobalObjectOwner> owner;
+      if(  
NS_SUCCEEDED(globalObject->GetGlobalObjectOwner(getter_AddRefs(owner))) 
+	  && owner
+	 ) {

You can drop that error check, just do:

+      nsCOMPtr<nsIScriptGlobalObjectOwner> owner;
+      globalObject->GetGlobalObjectOwner(getter_AddRefs(owner));
+
+      if (owner) {
...

sr=jst

Comment 14

[Karsten Düsterloh]

Attachment: including jst's comments of #13

Comment 15

[Karsten Düsterloh]

Attachment: same as 138339, but with -w

Comment 16

[Karsten Düsterloh]

(taking from [e-mail] radar)

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 138339 [details] [diff] [review]
including jst's comments of #13

>+      nsCOMPtr<nsIScriptGlobalObjectOwner> owner;
>+      globalObject->GetGlobalObjectOwner(getter_AddRefs(owner));
>+      if(owner) {
>+        //send error event first, then proceed
>+        globalObject->GetDocShell(getter_AddRefs(docShell));
>+        if (docShell && !JSREPORT_IS_WARNING(report->flags)) {
>+          static PRInt32 errorDepth; // Recursion prevention
>+          errorDepth++;
>+
>+          nsCOMPtr<nsIPresContext> presContext;
>+          docShell->GetPresContext(getter_AddRefs(presContext));
>+
>+          if(presContext && errorDepth < 2) {

Non-conforming if( style -- please use if ( here to match prevailing style.

/be
>+          {
>+            if (owner)
>+                owner->ReportScriptError(errorObject);
>+            else
>+            { // dump error to the jsconsole even if no owner

That brace should go on the same line as the else, separated from it by a
space.	I think prevailing style goes further, and fully braces the then part
of the if when the else needs to be braced.

/be

Comment 18

[Karsten Düsterloh]

Attachment: Minor whitespace changes according to brendan's comment #17

The -w diff 138340 is essentially still valid.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 138342 [details] [diff] [review]
Minor whitespace changes according to brendan's comment #17

I just have nits.

>+      nsCOMPtr<nsIScriptGlobalObjectOwner> owner;
>+      globalObject->GetGlobalObjectOwner(getter_AddRefs(owner));
>+      if (owner) {
>+        //send error event first, then proceed

Not your fault, but space after the comment delimiter, and turn this into a
sentence for bonus points.


>+        globalObject->GetDocShell(getter_AddRefs(docShell));
>+        if (docShell && !JSREPORT_IS_WARNING(report->flags)) {
>+          static PRInt32 errorDepth; // Recursion prevention
>+          errorDepth++;

Since you're touching this, this really wants to be prefix instead of postfix
increment.

>+
>+          nsCOMPtr<nsIPresContext> presContext;
>+          docShell->GetPresContext(getter_AddRefs(presContext));
>+
>+          if (presContext && errorDepth < 2) {
>+            nsScriptErrorEvent errorevent;
>+            errorevent.eventStructType = NS_EVENT;
>+            errorevent.message = NS_SCRIPT_ERROR;
>+
>+            errorevent.fileName = fileName.get();
>+            errorevent.errorMsg = msg.get();
>+            errorevent.lineNr = report ? report->lineno : 0;
>+
>+            // HandleDOMEvent() must be synchronous for the recursion block
>+            // (errorDepth) to work.
>+            globalObject->HandleDOMEvent(presContext, &errorevent, nsnull,
>+                                         NS_EVENT_FLAG_INIT, &status);
>+          }
> 
>-        errorDepth--;
>-      }
>+          errorDepth--;

Prefix again.

>+        }
>+      } // have global Object Owner


This comment is useless if everything is properly indented, and there are a
plethora of editors which have brace matching so that this just gets in the
way.  Comments should be additive, not redundant.


> 
>       if (status != nsEventStatus_eConsumeNoDefault) {
>-
>         // Make an nsIScriptError and populate it with information from
>         // this error.
>         nsCOMPtr<nsIScriptError>
>@@ -241,7 +238,16 @@ NS_ScriptErrorReporter(JSContext *cx,
>           }
> 
>           if (NS_SUCCEEDED(rv))
>-            owner->ReportScriptError(errorObject);
>+          {

Brace must go on the same line as the if.


>+            if (owner)

Even one liners get braces.

>+                owner->ReportScriptError(errorObject);
>+            else { // dump error to the jsconsole even if no owner

Comment on its own line, and again be additive, not redundant.	"We lack an
owner to report this error to, so let's just report it to the console service
so we don't lose it."  Also one note about your comment, the nsIConsoleService
has no notion of js, so you should call it the console service and not the
jsconsole.


>+              nsCOMPtr<nsIConsoleService> 
>+                pConsoleService(do_GetService(NS_CONSOLESERVICE_CONTRACTID, &rv));

The 'p' variable prefix is used by some modules to denote a (raw?) pointer; but
not here.  Please drop it.
Additionally, when initialization does not fit on one line, use syntax like:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsJSEnvironmen
t.cpp&rev=1.211&mark=340-341,348-349#334


>+              if (NS_SUCCEEDED(rv))
>+                pConsoleService->LogMessage(errorObject);

Again, the one liner gets a brace.

>+            }
>+          }
>         }
>       }
>     }


r=caillon with those.

Comment 20

[Karsten Düsterloh]

Attachment: final version (I hope ;-) )

Addressed caillons comments.

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in, but I forgot to attribute the patch author.  Sorry, I didn't realize
that until after the fact.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 138356 [details] [diff] [review]
final version (I hope ;-) )

sr=jst on this version too! :-)