613364 - javascript strict errors fail to show

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[David Dahl :ddahl]

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.

Comment 1

[David Dahl :ddahl]

I discovered this while working on bug 611032, will post a WIP patch soon.

Comment 2

[David Dahl :ddahl]

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

Comment 3

[Kevin Dangoor]

Requesting blocking for this because strict errors are not being handled properly.

Comment 4

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 5

[David Dahl :ddahl]

This bug really should block as we miss some hard to debug corner case content errors

Comment 6

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

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.

Comment 7

[David Dahl :ddahl]

(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.

Comment 8

[Kevin Dangoor]

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.

Comment 9

[Mihai Sucan [:msucan]]

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.

Comment 10

[Kevin Dangoor]

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.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: proposed fix

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.

Comment 12

[David Dahl :ddahl]

Comment on attachment 498763 [details] [diff] [review]
proposed fix

Great catch! thanks. Looks good.

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 498763 [details] [diff] [review]
proposed fix

Thanks for the feedback+ David!

Asking for review.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Alternatively you could just include every combination, I suppose...

Comment 16

[Mihai Sucan [:msucan]]

(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.

Comment 17

[Mihai Sucan [:msucan]]

(In reply to comment #15)
> Alternatively you could just include every combination, I suppose...

That would be overkill...

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 19

[Mihai Sucan [:msucan]]

(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 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 498763 [details] [diff] [review]
proposed fix

r=me with the followup filed.

Comment 21

[Mihai Sucan [:msucan]]

Thanks for the review+!

I have already filed the followup bug 620576.

Comment 22

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch.

Comment 23

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/29cf1c5e83dd