Closed Bug 613364 Opened 11 years ago Closed 11 years ago

javascript strict errors fail to show

Categories

(DevTools :: General, defect, P1)

defect

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Firefox 4.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: ddahl, Assigned: msucan)

References

Details

Attachments

(1 file, 1 obsolete file)

JavaScript strict error: http://z-ecx.images-amazon.com/images/G/01/goldbox/client-side/deal_notifier-v7._V194170980_.js, line 368: assignment to undeclared variable Deal

Apparently, we have a typeStrict property in the properties file: 
http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/headsUpDisplay.properties#6 

but, at least some strict errors trigger:

WARNING: NS_ENSURE_TRUE(aName) failed: file /home/ddahl/code/moz/mozilla-central/mozilla-central/intl/strres/src/nsStringBundle.cpp, line 270
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIStringBundle.GetStringFromName]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: resource:///modules/HUDService.jsm :: HS_getStr :: line 1382"  data: no]


When I logged the arg 'aName' in HS_getStr, aName is undefined.
I discovered this while working on bug 611032, will post a WIP patch soon.
Assignee: nobody → ddahl
Blocks: devtools4
I need to write a test case to see if this is affecting the current code, or just the new lazy-console-related followup patches
Requesting blocking for this because strict errors are not being handled properly.
blocking2.0: --- → ?
mass change: filter on PRIORITYSETTING
Priority: -- → P1
This bug really should block as we miss some hard to debug corner case content errors
David, can we get a better explanation on this bug? We can't determine if this is blocking based on the summary and description here.
Whiteboard: [needs-more-information]
(In reply to comment #6)
> David, can we get a better explanation on this bug? We can't determine if this
> is blocking based on the summary and description here.

Apparantly, some nsIScriptErrors do not have a flag property indicating that it is a strict, error or warn error. 

This might actually be fixed by bug 606498 - but also I am staring to wonder if we are seeing an nsIConsoleMessage and treating it like an nsIScriptError.

Further investigation is needed.
I've cc'ed Mihai (who is responsible for bug 606498) to see if he thinks those patches will likely fix this problem, but I admit to still being unclear as to the severity of this issue.
Bug explanation: internally Gecko can issue nsIScriptError messages that have flags for error, warning or exception. Beyond that, we also have the combination of strict error/warning/exception. The way we check the nsIScriptError flags is not entirely correct, because the flags property can have the error/warning/exception bits + the strict bit that is set (or not).

We do not have all the possible combinations of flags in HUDService.scriptErrorFlags and HUDService.scriptMsgLogLevel. Thus, we get undefined behavior - which equates to the string bundle code throwing an error because we don't know what message to display.

I did a fix for this in the Web Console bug 601177, but back then I did not find a way to repro the case when a strict error was thrown - only a strict warning was tested. That case is definitely fixed.

I will look into fixing the strict error case as well, now that you provided an example of when it happens.
Assignee: ddahl → mihai.sucan
To clarify: this bug should be a blocker because the user impact is that many script errors will just not show up in the console because of this bug. Mihai is working on a fix.
Attached patch proposed fix (obsolete) — Splinter Review
This is the proposed fix.

I added two new flags (based on jsapi.h). The example you found has three flags (designated decimal number 13). This is a strict_error, an error and a warning, which is weird. I labeled it as a warning. Shall we change it to something else?

It's also possible that we are not catching all the possible flags combinations. Shall we add more combinations? Or shall we change the code to use a more flags-friendly approach than the current one? (see logConsoleActivity).

Looking forward to your feedback.
Attachment #498763 - Flags: feedback?(ddahl)
Status: NEW → ASSIGNED
Whiteboard: [needs-more-information] → [patchclean:1220]
Comment on attachment 498763 [details] [diff] [review]
proposed fix

Great catch! thanks. Looks good.
Attachment #498763 - Flags: feedback?(ddahl) → feedback+
Comment on attachment 498763 [details] [diff] [review]
proposed fix

Thanks for the feedback+ David!

Asking for review.
Attachment #498763 - Flags: review?(sdwilsh)
Summary: javascript strict errors do not use the property when creating a log message → javascript strict errors fail to show
blocking2.0: ? → betaN+
Comment on attachment 498763 [details] [diff] [review]
proposed fix

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+  // See jsapi.h (JSErrorReport flags):
>+  // http://mxr.mozilla.org/mozilla-central/source/js/src/jsapi.h#3429
>   scriptErrorFlags: {
>-    0: "error",
>-    1: "warn",
>-    2: "exception",
>-    4: "error", // strict error
>-    5: "warn", // strict warning
>+    0: "error", // JSREPORT_ERROR
>+    1: "warn", // JSREPORT_WARNING
>+    2: "exception", // JSREPORT_EXCEPTION
>+    4: "error", // JSREPORT_STRICT | JSREPORT_ERROR
>+    5: "warn", // JSREPORT_STRICT | JSREPORT_WARNING
>+    8: "error", // JSREPORT_STRICT_MODE_ERROR
>+    13: "warn", // JSREPORT_STRICT_MODE_ERROR | JSREPORT_WARNING | JSREPORT_ERROR

Hardcoding specific combinations seems fragile - what if someone starts using e.g. JSREPORT_STRICT_MODE_ERROR | JSREPORT_WARNING?

Shouldn't we instead take cascading-importance approach to bucketing these, by looking at all the flags?
Alternatively you could just include every combination, I suppose...
(In reply to comment #14)
> Comment on attachment 498763 [details] [diff] [review]
> proposed fix
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> 
> Hardcoding specific combinations seems fragile - what if someone starts using
> e.g. JSREPORT_STRICT_MODE_ERROR | JSREPORT_WARNING?
> 
> Shouldn't we instead take cascading-importance approach to bucketing these, by
> looking at all the flags?

Agreed. I kept the patch minimal in changes, for quick-review purposes.

Shall I change the code's approach to how it checks those flags? I can reproduce the jsapi approach - to hold the relevant JSREPORT_* constants and check their bits against the nsIScriptError.flags property.

I believe that would be the correct approach.
(In reply to comment #15)
> Alternatively you could just include every combination, I suppose...

That would be overkill...
Assuming the current patch covers all known cases, I think it's fine to take for now (easy to review). But we should get a followup filed on the cleaner solution.
(In reply to comment #18)
> Assuming the current patch covers all known cases, I think it's fine to take
> for now (easy to review). But we should get a followup filed on the cleaner
> solution.

We assumed, until now, we know all the cases. This is a new case - which is weird, because it combines error *and* warning. We know of no other cases, at the moment. Will file a follow up bug.
Comment on attachment 498763 [details] [diff] [review]
proposed fix

r=me with the followup filed.
Attachment #498763 - Flags: review?(sdwilsh) → review+
Thanks for the review+!

I have already filed the followup bug 620576.
Whiteboard: [patchclean:1220] → [patchclean:1220][checkin]
Attached patch rebased patchSplinter Review
Rebased patch.
Attachment #498763 - Attachment is obsolete: true
Whiteboard: [patchclean:1220][checkin] → [patchclean:0103][checkin]
http://hg.mozilla.org/mozilla-central/rev/29cf1c5e83dd
Whiteboard: [patchclean:0103][checkin]
Target Milestone: --- → Firefox 4.0b9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.