Closed
Bug 229437
Opened 21 years ago
Closed 21 years ago
"WARNING: Failed to get a global Object Owner" suppresses actual JS error message
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: mnyromyr, Assigned: mnyromyr)
Details
Attachments
(1 file, 9 obsolete files)
4.28 KB,
patch
|
jst
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•21 years ago
|
||
Instead of just returning when having no global object owner, only dump to the console
![]() |
||
Comment 3•21 years ago
|
||
The problem is that nsXULPrototypeDocument::NewXULPDGlobalObject uses a shared global object with no owner for all chrome documents...
Assignee | ||
Comment 4•21 years ago
|
||
@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. :|
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•21 years ago
|
||
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
Assignee: general → dom_bugs
Component: JavaScript Engine → DOM
Comment 7•21 years ago
|
||
Brendan: the default owner for dom bugs is general@dom.bugs, dom_bugs@propagandism.org is an account I use for watching it. I can rename it if you want to avoid confusion.
Assignee: dom_bugs → general
QA Contact: PhilSchwartau → ian
Assignee | ||
Comment 8•21 years ago
|
||
Handle a missing owner gracefully and dump the error message now both to jsconsole and console.
Assignee | ||
Updated•21 years ago
|
Attachment #137976 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #138018 -
Flags: review?(timeless)
Comment 10•21 years ago
|
||
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.
Attachment #138019 -
Flags: superreview-
Assignee | ||
Comment 11•21 years ago
|
||
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.)
Assignee | ||
Updated•21 years ago
|
Attachment #138018 -
Attachment is obsolete: true
Attachment #138019 -
Attachment is obsolete: true
Assignee | ||
Comment 12•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #138018 -
Flags: review?(timeless)
Assignee | ||
Updated•21 years ago
|
Attachment #138303 -
Flags: superreview?(jst)
Attachment #138303 -
Flags: review?(timeless)
Comment 13•21 years ago
|
||
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
Attachment #138303 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 14•21 years ago
|
||
Attachment #138303 -
Attachment is obsolete: true
Attachment #138304 -
Attachment is obsolete: true
Assignee | ||
Comment 15•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #138303 -
Flags: review?(timeless)
Assignee | ||
Updated•21 years ago
|
Assignee: general → mnyromyr
Status: ASSIGNED → NEW
Assignee | ||
Updated•21 years ago
|
Attachment #138339 -
Flags: review?(caillon)
Comment 17•21 years ago
|
||
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
Assignee | ||
Updated•21 years ago
|
Attachment #138339 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #138339 -
Flags: review?(caillon)
Assignee | ||
Updated•21 years ago
|
Attachment #138342 -
Flags: review?(caillon)
Comment 19•21 years ago
|
||
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.
Attachment #138342 -
Flags: review?(caillon) → review+
Assignee | ||
Comment 20•21 years ago
|
||
Addressed caillons comments.
Attachment #138340 -
Attachment is obsolete: true
Attachment #138342 -
Attachment is obsolete: true
Comment 21•21 years ago
|
||
Checked in, but I forgot to attribute the patch author. Sorry, I didn't realize that until after the fact.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 22•21 years ago
|
||
Comment on attachment 138356 [details] [diff] [review] final version (I hope ;-) ) sr=jst on this version too! :-)
Attachment #138356 -
Flags: superreview+
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•