Closed Bug 1094057 Opened 6 years ago Closed 5 years ago

Violations of "use strict"; should generate errors, not warnings

Categories

(Core :: JavaScript Engine, defect)

33 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: peter.kehl, Assigned: bugzilla)

References

Details

(Keywords: testcase)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140923175406

Steps to reproduce:

Have a privileged script, that runs in strict mode ("use strict";). Have an error in that script that violates strict mode. See attached XUL, which has three various violations.


Actual results:

The script stops (as it should). However, there is no message in Browser Console, if in 'JS' tab/dropdown you disable level 'Warnings'. It only shows a message when you enable 'Warnings'.


Expected results:

Since such violation of "use strict"; stop JS script from being executed, they should show up in Browser Console as errors and not as warnings.

Currently level 'Warnings' is confusing. Say you have a script which makes no immediate obvious change. And say you disable 'Warnings', because other add-ons (or even Firefox itself) generate warnings that you're not interested in. Then you may not notice a violation of strict mode in your file for a while.
OS: Windows 7 → Linux
Version: 32 Branch → 33 Branch
Attached file killerWarnings.xul
I've searched bugzilla a few times. The closest bugs I've found are https://bugzilla.mozilla.org/show_bug.cgi?id=823310 and https://bugzilla.mozilla.org/show_bug.cgi?id=266269. They don't seem related to this.
Component: Untriaged → JavaScript Engine
Keywords: testcase
Product: Firefox → Core
To save your time, I was checking for potential duplicates. Another closest one is https://bugzilla.mozilla.org/show_bug.cgi?id=226293. I think they're not related, since https://bugzilla.mozilla.org/show_bug.cgi?id=226293 mentions preferences for strict warnings (i.e. not "use strict";).
See Also: → 1096135
Bug 1094057 - Violations of "use strict"; should generate errors, not warnings

Make the use of JSREPORT_STRICT consistent across all places where it is used.
I.e. never add JSREPORT_STRICT when reporting JSREPORT_ERROR in strict mode,
but only when reporting JSREPORT_WARNING in non-strict mode.
This happens to make the errors appear as errors instead of warnings in the console.
Comment on attachment 8685590 [details]
MozReview Request: Bug 1094057 - Violations of "use strict"; should generate errors, not warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24845/diff/1-2/
Attachment #8685590 - Flags: review?(jorendorff)
Note that it may be possible to fix this on the devtools side instead of the JS engine side, by changing code like this:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#1483
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/DeveloperToolbar.jsm#697

But I have no idea how to find all instances if this in the devtools, therefore I opted to make the patch in the JS engine. Also, since the JS engine was already inconsistent in when it included JSREPORT_STRICT it seems like the better place to fix it.
Attachment #8685590 - Flags: review?(jorendorff) → review+
Comment on attachment 8685590 [details]
MozReview Request: Bug 1094057 - Violations of "use strict"; should generate errors, not warnings

https://reviewboard.mozilla.org/r/24845/#review22851

Much better. Watch out for breakage. :-\
It has been a few years since I last made a Firefox patch, so I may not be up to date with the process. Is there anything I need to do now?
(In reply to Jesper Kristensen from comment #8)
> It has been a few years since I last made a Firefox patch, so I may not be
> up to date with the process. Is there anything I need to do now?

We should push it to try considering how likely it is this will break things. Do you have level 1 commit access to do this yourself? If not, please ping me or jorendorff or someone in #developers and we can push it for you.
Flags: needinfo?(bugzilla)
I know I have l10n level access, so I tried, and it seems it includes access to try.

The various documentation I could find seems to contradict each other on what I should do. It seems like they were written at different points in time when different methods were recommended.

I ended up with this, which I hope is ok:

$ hg push-to-try -m "try: -b do -p linux -u all -t none"
Creating temporary commit for remote...
pushing to ssh://hg.mozilla.org/try
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 4 changes to 4 files (+1 heads)
remote: recorded push in pushlog
remote: replication of changegroup data completed successfully in 14.3s
remote:
remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/b062cd0f123b
remote:   https://hg.mozilla.org/try/rev/a3da96b7130e
remote:
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3da96b7130e
push complete
temporary commit removed, repository restored
Flags: needinfo?(bugzilla)
(In reply to Jesper Kristensen from comment #10)
> I know I have l10n level access, so I tried, and it seems it includes access
> to try.
> 
> The various documentation I could find seems to contradict each other on
> what I should do. It seems like they were written at different points in
> time when different methods were recommended.
> 
> I ended up with this, which I hope is ok:
> 
> $ hg push-to-try -m "try: -b do -p linux -u all -t none"

This is a good start, but you probably want -p all. I know it's frowned upon, but this change will affect all our frontend code - desktop, android and b2g - and just testing linux desktop likely won't be enough.

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3da96b7130e

This already brought up one issue though:

 263 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_strict_mode_errors.js | expectUncaughtException was called but no uncaught exception was detected! - 

(dt8 in the opt build)
I made a fix for my test, and am trying again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3de09c01b57b
(In reply to Jesper Kristensen from comment #12)
> I made a fix for my test, and am trying again:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3de09c01b57b

I checked, this looks OK, so I pushed it:

remote:   https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=be035c256991
Assignee: nobody → bugzilla
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
https://hg.mozilla.org/mozilla-central/rev/be035c256991
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.