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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Peter Lyons Kehl]

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.

Comment 1

[Peter Lyons Kehl]

Attachment: killerWarnings.xul

Comment 2

[Peter Lyons Kehl]

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.

Comment 3

[Peter Lyons Kehl]

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";).

Comment 4

[Jesper Kristensen]

Attachment: MozReview Request: Bug 1094057 - Violations of "use strict"; should generate errors, not warnings

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 5

[Jesper Kristensen]

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/

Comment 6

[Jesper Kristensen]

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.

Comment 7

[Jason Orendorff [:jorendorff]]

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. :-\

Comment 8

[Jesper Kristensen]

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?

Comment 9

[:Gijs (he/him)]

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

Comment 10

[Jesper Kristensen]

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

Comment 11

[:Gijs (he/him)]

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

Comment 12

[Jesper Kristensen]

I made a fix for my test, and am trying again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3de09c01b57b

Comment 13

[:Gijs (he/him)]

(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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be035c256991