Closed Bug 821103 Opened 7 years ago Closed 7 years ago

don't display strict warnings when not in strict mode

Categories

(Core :: JavaScript Engine, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: Benjamin, Assigned: Benjamin)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
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.
This makes StrictModeGetter's results correct in the BytecodeEmitter.
Attachment #691631 - Flags: review?(n.nethercote)
This fixes the bug.
Attachment #691632 - Flags: review?(n.nethercote)
Here's the same thing with a little testcase I meant to add.
Attachment #691631 - Attachment is obsolete: true
Attachment #691631 - Flags: review?(n.nethercote)
Attachment #691636 - Flags: review?(n.nethercote)
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?
(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 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?
Attachment #691636 - Flags: review?(n.nethercote) → review-
(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.
> 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.
(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 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.
Blocks: 821470
Raising this bug's severity to major - the patches here will fix crashes that are constantly hit by the js fuzzers in bug 821470.
Severity: normal → major
No longer blocks: 821470
Duplicate of this bug: 821470
(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.
Please put in the testcases for bug 821470 when you land this bug.
Flags: in-testsuite?
(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.
Duplicate of this bug: 821970
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.
Attachment #691632 - Flags: review?(n.nethercote) → review+
> > 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.
(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.
I should note the current patch also handles both the NULL and incorrect scope cases.
(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".
> 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.
(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?
> 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.
Attachment #692837 - Flags: review?(benjamin)
Assignee: general → n.nethercote
> 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.
Assignee: n.nethercote → benjamin
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()|.
Attachment #692837 - Flags: review?(benjamin) → review+
Attachment #691636 - Attachment is obsolete: true
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.
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.
Attachment #691632 - Attachment is obsolete: true
Attachment #692942 - Flags: review?(n.nethercote)
Attachment #692837 - Flags: checkin+
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.
Attachment #692942 - Flags: review?(n.nethercote) → review+
> 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 :)
(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?
You can use options() to set options.  See jit-test/tests/basic/bug657975.js for an example that turns on strict warnings.
https://hg.mozilla.org/mozilla-central/rev/eb8734ce0dc2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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/compatibility@addons.mozilla.org.xpi!/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?
> 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.
(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.
roc: that's XDR_BYTECODE_VERSION in js/src/vm/Xdr.h.
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.
I don't understand exactly why, but the warning spew actually makes my debug build unusable.
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?)
roc, I think you may be seeing bug 823310. Can you see if the patch there fixes your problems?
You need to log in before you can comment on or make changes to this bug.