821103 - don't display strict warnings when not in strict mode

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

We shouldn't display strict warnings when not in strict mode and cx->hasStrictOption() is not true. Bug 819509 broke this when I refactored some code in TokenStream::reportCompileErrorNumberVA.

Comment 1

[:Benjamin Peterson]

The whole strict warning report mechanism quite a mess even besides this bug. For example, when the bytecode emitter calls TokenStream::reportStrictError, the TokenStream can call into StrictModeGetter which isn't valid outside of parsing.

Comment 2

[:Benjamin Peterson]

Attachment: Make StrictModeGetter work with SharedContext instead of Parser

This makes StrictModeGetter's results correct in the BytecodeEmitter.

Comment 3

[:Benjamin Peterson]

Attachment: don't raise strict warning for non-strict code

This fixes the bug.

Comment 4

[:Benjamin Peterson]

Attachment: Make StrictModeGetter work with SharedContext instead of Parser

Here's the same thing with a little testcase I meant to add.

Comment 5

[Nicholas Nethercote [inactive]]

Oh god I hate the strict warnings code.


> We shouldn't display strict warnings when not in strict mode and
> cx->hasStrictOption() is not true.

Is that right?  I can't even remember what the conditions for strict warnings are.  Can you give an example that changed behaviour as a result of bug 819509?

Comment 6

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> Oh god I hate the strict warnings code.

Me, too. Maybe the fact that its constantly broken without people noticing means we can kill some of it. At least cx->hasStrictOption()?

> 
> 
> > We shouldn't display strict warnings when not in strict mode and
> > cx->hasStrictOption() is not true.
> 
> Is that right?  I can't even remember what the conditions for strict
> warnings are.  Can you give an example that changed behaviour as a result of
> bug 819509?

Running this thing: https://octane-benchmark.googlecode.com/svn/latest/gbemu.js
You get warnings like

/home/benjamin/junk/gbemu.js:1347:0 strict warning: useless expression:
/home/benjamin/junk/gbemu.js:1347:0 strict warning: "use strict";
/home/benjamin/junk/gbemu.js:1347:0 strict warning: ^
/home/benjamin/junk/gbemu.js:10685:0 strict warning: useless expression:
/home/benjamin/junk/gbemu.js:10685:0 strict warning: "use strict";
/home/benjamin/junk/gbemu.js:10685:0 strict warning: ^

but the file doesn't run in strict mode.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 691632 [details] [diff] [review]
don't raise strict warning for non-strict code

Review of attachment 691632 [details] [diff] [review]:
-----------------------------------------------------------------

There's also a Parser::reportStrictWarning() that presumably should be modified to call TokenStream::reportStrictWarningErrorNumberVA.

::: js/src/frontend/TokenStream.cpp
@@ +437,1 @@
>          return true;

I don't understand this.  If strictMode() is true we used to set JSREPORT_ERROR and now we don't.  Is that the point?

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to :Benjamin Peterson from comment #1)
> The whole strict warning report mechanism quite a mess even besides this
> bug. For example, when the bytecode emitter calls
> TokenStream::reportStrictError, the TokenStream can call into
> StrictModeGetter which isn't valid outside of parsing.

I don't understand.  When the BytecodeEmitter is running the Parser and TokenStream are still alive and usable.  Why isn't StrictModeGetter valid?

I don't understand the patch.  It looks much uglier and more error-prone than what we currently have.

I also don't understand what the test is doing.  What's the |ieval| for?  When I compile the function with a current shell I get this:

  a.js:3:0 strict warning: useless expressio:
  a.js:3:0 strict warning: function x() { 'use strict'; const x = 4; x = 3; }

Overly terse patch descriptions lead to confused reviewers and waste everybody's time.

Comment 9

[Nicholas Nethercote [inactive]]

> Overly terse patch descriptions lead to confused reviewers and waste
> everybody's time.

To elaborate on that a bit:  you keep writing (short) descriptions of what's wrong, but don't describe what the patch does to fix it.

Comment 10

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> (In reply to :Benjamin Peterson from comment #1)
> > The whole strict warning report mechanism quite a mess even besides this
> > bug. For example, when the bytecode emitter calls
> > TokenStream::reportStrictError, the TokenStream can call into
> > StrictModeGetter which isn't valid outside of parsing.
> 
> I don't understand.  When the BytecodeEmitter is running the Parser and
> TokenStream are still alive and usable.  Why isn't StrictModeGetter valid?

Because the ParseContext is pointing to the global SharedContext not to a particular function which the BytecodeEmitter might be emitting.

> 
> I don't understand the patch.  It looks much uglier and more error-prone
> than what we currently have.

Well, what we currently have is also wrong. Either we need something like this or the BytecodeEmitter shouldn't report strict errors through the tokenizer.

> 
> I also don't understand what the test is doing.  What's the |ieval| for? 

It's actually superfluous. It just helped me narrow down the cause when I was debugging it.

> When I compile the function with a current shell I get this:
> 
>   a.js:3:0 strict warning: useless expressio:
>   a.js:3:0 strict warning: function x() { 'use strict'; const x = 4; x = 3; }

Exactly. That warning is wrong and a strict error about a constant being reassigned should be emitted instead.

Comment 11

[:Benjamin Peterson]

Comment on attachment 691632 [details] [diff] [review]
don't raise strict warning for non-strict code

Review of attachment 691632 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/TokenStream.cpp
@@ +437,1 @@
>          return true;

This is semantically equivalent; it's just a small cleanup. JSREPORT_ERROR = 0 (!), so setting it is unecessary.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Raising this bug's severity to major - the patches here will fix crashes that are constantly hit by the js fuzzers in bug 821470.

Comment 14

[:Benjamin Peterson]

(In reply to :Benjamin Peterson from comment #10)
> (In reply to Nicholas Nethercote [:njn] from comment #8)
> > I don't understand the patch.  It looks much uglier and more error-prone
> > than what we currently have.
> 
> Well, what we currently have is also wrong. Either we need something like
> this or the BytecodeEmitter shouldn't report strict errors through the
> tokenizer.

We could also due away with the StrictModeGetter abstraction and simply set a strictness flag on the TokenStreamer if you think that's nicer.

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Please put in the testcases for bug 821470 when you land this bug.

Comment 16

[:Benjamin Peterson]

(In reply to Gary Kwong [:gkw] from comment #15)
> Please put in the testcases for bug 821470 when you land this bug.

Testcase in these patches is equivalent.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 691632 [details] [diff] [review]
don't raise strict warning for non-strict code

Review of attachment 691632 [details] [diff] [review]:
-----------------------------------------------------------------

To elaborate further on how to make things easy for your reviewer.
- Explain what's wrong with the current code.  Examples are great.
- Explain what changes your patch makes, and how it fixes the problem.
- Don't combine refactoring changes with changes that affect behaviour.  Or if you do, make it clear which parts are which.  (I think it's the condition in TokenStream::reportStrictWarningErrorNumberVA, but I'm still not certain about that.)

::: js/src/frontend/TokenStream.cpp
@@ +437,1 @@
>          return true;

Ugh.  That's horribly non-obvious and fragile.  r=me if you put it back in, even though it's unnecessary with the current value.

Comment 19

[Nicholas Nethercote [inactive]]

> > I don't understand.  When the BytecodeEmitter is running the Parser and
> > TokenStream are still alive and usable.  Why isn't StrictModeGetter valid?
> 
> Because the ParseContext is pointing to the global SharedContext not to a
> particular function which the BytecodeEmitter might be emitting.

I ran the test from bug 821470 through the debugger and the problem is that parser->pc is null, and gets dereferenced.

> Either we need something like
> this or the BytecodeEmitter shouldn't report strict errors through the
> tokenizer.

It was working fine before bug 819509.  The problem is that CompileFunctionBody used to create a local ParseContext, and its constructor sets |parser->pc| to point to it.  That could then be used by the BCE, because ParseContext's lifetime spans BCE's lifetime.

In bug 819509 you moved the ParseContext into standaloneFunctionBody and it's lifetime is now less than the BCE.  Sorry I didn't catch this in the review.  I suggest moving the ParseContext back into CompileFunctionBody.  You'll have to do likewise with the FunctionBox.  In fact, I'd recommend undoing the changes from bug 819509 from CompileFunctionBody as much as possible, and just making the smallest possible changes, because the state management here is, as we have seen, quite subtle.

Comment 20

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> In bug 819509 you moved the ParseContext into standaloneFunctionBody and
> it's lifetime is now less than the BCE.  Sorry I didn't catch this in the
> review.  I suggest moving the ParseContext back into CompileFunctionBody.

I don't think this is completely correct. The parse context won't be NULL then, but the "wrong" one could still be used. For example:

Function("
    function f() {
        'use strict';
        const x = 4;
        x = 5;
    }
")

would incorrectly give only a warning because the ParseContext the TokenStream would see would be the function-level non-strict one.

Comment 21

[:Benjamin Peterson]

I should note the current patch also handles both the NULL and incorrect scope cases.

Comment 22

[:Benjamin Peterson]

(In reply to :Benjamin Peterson from comment #20)
> would incorrectly give only a warning because the ParseContext the
> TokenStream would see would be the function-level non-strict one.

More specifically "toplevel Function non-strict one".

Comment 23

[Nicholas Nethercote [inactive]]

> I should note the current patch also handles both the NULL and incorrect
> scope cases.

Sure, but it's papering over the problem.  If the ParseContext seen by TokenStream is wrong, we should fix that, rather than handling separately the one value we happen to pull out of the ParseContext for these failing cases.

Comment 24

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #23)
> > I should note the current patch also handles both the NULL and incorrect
> > scope cases.
> 
> Sure, but it's papering over the problem.  If the ParseContext seen by
> TokenStream is wrong, we should fix that, rather than handling separately
> the one value we happen to pull out of the ParseContext for these failing
> cases.

I don't follow. The ensures TokenStream does see the right SharedContext. Where's the special-casing?

Comment 25

[Nicholas Nethercote [inactive]]

Attachment: Avoid calling TokenStream::inStrictMode() from the BytecodeEmitter.

> The ensures TokenStream does see the right SharedContext.

Here's what I meant:  parser->pc is wrong, which means that
parser->pc->sc->strict is wrong.  You haven't fixed parser->pc, you've just
avoided the need to access parser->pc.  That's what I don't like about it.

----

The attached patch is an alternative approach.  It requires callers of
TokenStream::reportStrictModeErrorNumberVA to pass in a |strictMode| argument.
This prevents TokenStream::inStrictMode() being called when we're in the BCE
and parser->pc isn't accessible.  It fixes the test in your patch and also the
test in bug 821470 comment 0.

I like this approach better because it's hard to get wrong -- there's no need
to remember to set a piece of state in several different places.

It doesn't fix the fact that parser->pc shouldn't be accessed when we're in
the BCE, but I'll try to fix that in a follow-up bug.

Comment 26

[Nicholas Nethercote [inactive]]

> The ensures TokenStream does see the right SharedContext.

Here's what I meant:  parser->pc is wrong, which means that parser->pc->sc->strict is wrong.  You haven't fixed parser->pc, you've just avoided the need to access parser->pc.  That's what I don't like about it.

Comment 27

[:Benjamin Peterson]

Comment on attachment 692837 [details] [diff] [review]
Avoid calling TokenStream::inStrictMode() from the BytecodeEmitter.

Review of attachment 692837 [details] [diff] [review]:
-----------------------------------------------------------------

You're right. This is much better. I hope we can arrange for TokenStream::strictMode() to be poisoned when it's not being used by the Parser. Also or alternatively, the convience methods which don't take a |strictMode| argument could be made private to the TokenStream, since the Parser has its own convenience methods.

Nit on the commit message: the method is |strictMode()| not |inStrictMode()|.

Comment 28

[:Benjamin Peterson]

Since this was causing fuzzing problems, I landed njn's fix. (njn's patch went to bug 821470 because it directly fixes the issue there.) I'm going to file a followup bug to poision TokenStream:;strictMode() outside the parser.

Comment 29

[:Benjamin Peterson]

Attachment: don't raise strict warning for non-strict code

Here is an updated patch for not giving strict warnings when strict mode is not enabled ala njn's patch for fixing the crash; we explicitly pass strictness from the BytecodeEmitter to the error handling code.

Comment 30

[Nicholas Nethercote [inactive]]

Comment on attachment 692942 [details] [diff] [review]
don't raise strict warning for non-strict code

Review of attachment 692942 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if you also convert Parser::reportStrictWarning to call reportStrictWarningErrorNumberVA.  Also, a test would be great.

Comment 31

[Nicholas Nethercote [inactive]]

> I hope we can arrange for
> TokenStream::strictMode() to be poisoned when it's not being used by the
> Parser. Also or alternatively, the convience methods which don't take a
> |strictMode| argument could be made private to the TokenStream, since the
> Parser has its own convenience methods.

Yeah, both would be good.  I thought about the poisoning yesterday and couldn't come up with a way that was clean.  I'll let you know if I come up with anything today.

Thanks for the fixes.  Glad we got there in the end :)

Comment 32

[:Benjamin Peterson]

(In reply to Nicholas Nethercote [:njn] from comment #30)
> Comment on attachment 692942 [details] [diff] [review]
> don't raise strict warning for non-strict code
> 
> Review of attachment 692942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if you also convert Parser::reportStrictWarning to call
> reportStrictWarningErrorNumberVA.  Also, a test would be great.

Do you know how we can test non-standard shell options, or indeed warnings at all?

Comment 33

[Nicholas Nethercote [inactive]]

You can use options() to set options.  See jit-test/tests/basic/bug657975.js for an example that turns on strict warnings.

Comment 34

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8734ce0dc2

Comment 35

[Ed Morley [:emorley]]

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

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I made a build that includes this changeset, but I'm still getting strict-mode (I assume) warnings on startup, e.g.

JS Component Loader: WARNING resource://gre/modules/XPIProvider.jsm -> jar:file:///C:/Users/roc/AppData/Roaming/Mozilla/Firefox/Profiles/qibi2ljp.Test/extensions/[e-mail]!/bootstrap.js -> resource://compatibility-at-addons-dot-mozilla-dot-org/api-utils/lib/loader.js -> resource://compatibility-at-addons-dot-mozilla-dot-org/api-utils/lib/sandbox.js -> resource://compatibility-at-addons-dot-mozilla-dot-org/acr/data/lib/jquery-1.7.2.min.js:2
                     function ci does not always return a value

Is there some state persisted in the profile that I need to clear?

Comment 37

[Nicholas Nethercote [inactive]]

> Is there some state persisted in the profile that I need to clear?

Strictness is stored in XDR'd scripts, so maybe that's the problem?  But I don't know how to clear that.

Comment 38

[Jeff Walden [:Waldo]]

(In reply to Nicholas Nethercote [:njn] from comment #37)
> But I don't know how to clear that.

An expedient hack: bump the XDR version.

Comment 39

[Nicholas Nethercote [inactive]]

roc: that's XDR_BYTECODE_VERSION in js/src/vm/Xdr.h.

Comment 40

[Jeff Walden [:Waldo]]

Wait.  We're not suggesting he bump the version himself, surely!  If we really want proof positive of this, we should be bumping it ourselves, or landing some other stupid patch that bumps it.  It is *not* a good idea to bump it yourself, because you'll only get a conflict when one of us does, eventually, or you'll bump it, then we'll bump it and your bump will merge with ours and you'll get old/new bytecode mismatches and possibly crashing hilarity.

So, if we want to clear this, we should bump it ourselves.  No one else should be doing so.  I don't much care which way we go, myself.  But surely a few warnings until the next bump won't hurt anyone that much.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't understand exactly why, but the warning spew actually makes my debug build unusable.

Comment 42

[:Benjamin Peterson]

I don't think this is an issue with XDR scripts. The warning roc has showed us is generated by the compiler, which means we're not loading from XDRing. Anyway, the bug was never that we marked some scripts as script that shouldn't be rather we simply emit incorrect warnings on them. That said, I don't know what else it would be. (Could javascript.options.strict be accidently bumped on?)

Comment 43

[:Benjamin Peterson]

roc, I think you may be seeing bug 823310. Can you see if the patch there fixes your problems?