807862 - Use strict mode for all builtin JS

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Tracking, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should run all builtin JS in strict mode.  CCing some people who might be interested.

Comment 1

[Justin Dolske [:Dolske]]

Care to expand on why we should do that?

Comment 2

[Nicholas Nethercote [inactive]]

Partly because it'll help with enabling the patch in bug 798491, and partly because it catches lots of common JS defects.

Comment 3

[Justin Dolske [:Dolske]]

Fair 'nuff. I don't think I have any objection to starting to fix stuff piece by piece. I know the addon manager already uses strict mode, and now that I look I'm surprised by how much other code does too.

Two tiny exceptions:

1) AIUI there can be some subtle breakage if existing code is doing dumb things (which I would hardly be shocked to find in our code ;). Let's just be careful.

2) I'm pretty sure there will be some addon-compat issues with doing this in code that addons twiddle directly (possibly other cases?). Mostly browser.js, but I don't have a good sense for how big a deal it would be yet. But I'd want to be reasonably cautious in such areas. [I see that 'use strict' can be used in functions, as well as whole-scripts... That might be a path for slow change, as well as preparing new-code.]

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I converted (nearly) all Add-ons Manager code to use strict mode in bug 670023, there's some history there as to the kind of things (bugs!) the conversion caught, and additional reasons for using strict mode (such as performance!).

I say "nearly all" because I didn't do XBL bindings - the only way to do that currently is add "use strict"; to every single function. Bug 669190 would give us the ability to do it for entire <binding>s.

When reviewing, I now insist that all new modules & js files use strict mode (some other reviews are doing the same) - that's been helping a lot.

Around the same time I'd briefly looked into making browser.js use strict mode - I remember there being some bad code patterns that would need cleaned up (eg, usage of the arguments pseudo-array, functions declared in conditionals) - all fixable stuff. 

But add-on compat was a bit of a concern, due to the way strict mode and non-strict mode interact. The details escape me at the moment, but there's something that can be broken in non-strict mode function (eg, add-on code) if its called by strict mode code (eg, browser code). If I could remember what that was, I could comment on how likely a problem that would be :\

Comment 5

[Nicholas Nethercote [inactive]]

> The details escape me at the moment, but there's
> something that can be broken in non-strict mode function (eg, add-on code)
> if its called by strict mode code (eg, browser code).

I bet Waldo will know...

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Coincidentally, this I just noticed this came up on the es-discuss list recently. Consider the following:

  // browser.js
  function browser_func() {
    "use strict";
    addon_func();
  }

  // add-on code
  function addon_func() {
    arguments.callee.caller;
  }

  browser_func();

  /*
  Exception: access to strict mode caller function is censored
  */


Looking at the addons mxr, there are about 75 add-ons that use arguments.callee.caller. I don't know how many of those would be affected, or stats of their usage (my little tool for gathering addon usage stats from mxr doesn't work for this, sadly). But that's not as bad as I expected.

Not sure if there are other types of strict-mode poisoning that would have an impact here.

Comment 7

[Nicholas Nethercote [inactive]]

> ... arguments.callee.caller ...

Ah, that reminds me of something Kris wrote (in bug 712733 comment 20):

> There are other traps as well. Popular extensions like Tab Mix Plus, for
> instance, tends to check arguments.callee.caller.caller to decide whether it's
> being called by its own code. Add-ons in strict mode which try to call
> gBrowser.addTab will explode when used with Tab Mix Plus, and as any add-on
> developer knows, conflicts with Tab Mix Plus cause an endless flood of hate
> mail.

Comment 8

[Jeff Walden [:Waldo]]

Caller access, or function.arguments access, is the thing that seems most likely to bite stuff.

Past that, stupid addons that monkeypatch by recompiling from source could get bitten by different strict mode semantics -- things like |this| being exactly the value passed in, not something boxed (and in particular, for a call foo(), |this === undefined|), maybe one or two other things.  I know at one point we shoved a "use strict" string into such decompilations, to attempt to preserve strict semantics.  But that has its own problems (not to mention that the |function() expr| syntax doesn't support it), because then someone monkeypatching in a |with| is going to get hurt.  I'm not sure if we shoehorn in a "use strict" or not these days, what with returning saved source now.

All this said, it does seem a good thing to me to transition over to strict mode code.  Might take some effort to deal with addon issues, perhaps, but I think we could work through them with some patience.

Comment 9

[Nicholas Nethercote [inactive]]

about:memory and about:compartments are implemented in strict JS code, FWIW.  The stricter checking has saved me from trouble several times.  Fortunately that code has no interactions with add-ons (at least, none that I know of).

Comment 10

[Kris Maglione [:kmag]]

FWIW, I'm in favor of this for builtin code. That comment was in the context of recommending it for add-ons, where those issues could lead to a compatibility minefield. If those issues happen because of interaction with core code, we don't have to worry as much about the authors of affected add-ons not noticing, or not caring enough to fix the problems (although it seems that the Tab Mix Plus devs haven't cared enough to fix their current release after Function.toString changed).

We just need to make sure that this lands in nightly and rides the trains to give them time to adapt, and that we add compatibility checks to the validator.

As for the decompiler sticking "use strict" at the head of decompiled functions, I wouldn't worry so much about breaking monkey patchers who tried to use with statements and the like. The practice is bad enough without adding more misfeatures to the mix, and I'd be happy to break it. However, I suspect it's more likely that they'd just s/// out the "use strict" statement.

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Caller access, or function.arguments access, is the thing that seems most
> likely to bite stuff.

To address the problem Blair demonstrated in comment 6, can we materialize an `arguments.callee.caller` by inserting non-strict thunk functions where our strict builtin JS calls out to addons' JS?

Comment 12

[Kris Maglione [:kmag]]

(In reply to Chris Peterson (:cpeterson) from comment #11)
> To address the problem Blair demonstrated in comment 6, can we materialize
> an `arguments.callee.caller` by inserting non-strict thunk functions where
> our strict builtin JS calls out to addons' JS?

If we do, we should modify the deprecated `arguments` properties so that they display a deprecation warning. I'd rather stop add-ons relying on this kind of behavior than support it long term.

In any case, I don't think that's feasible. Most such uses are from monkey patching, or similar circumstances where we can't know we're calling add-on code. It also doesn't help for things like `arguments.callee.caller.caller`, which do exist.

Comment 13

[Kris Maglione [:kmag]]

Add-ons MXR is back up, and a search for arguments.callee.caller.caller reveals such gems as:


line 3472 -- && arguments.callee.caller.caller
line 3473 -- && arguments.callee.caller.caller.caller
line 3474 -- && arguments.callee.caller.caller.caller.name == "sss_duplicateTab")

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: extra-warnings.patch

As a stepping stone to enabling ES5 strict mode for all builtin JS, we could enable SpiderMonkey's "extra warnings" mode (as errors) in release builds. "Extra warnings" mode (AKA "javascript.options.strict warnings", not to be confused with ES5 "strict mode") is currently enabled (not as errors) in debug builds, but some frontend developers and testers might not run debug builds.

Extra warnings mode reports many of the same issues as ES5 strict mode, but without any semantic changes (like arguments.callee.caller problem discussed above).

This patch reports some harmless issues in browser components and devtools, plus some real bugs and dead code in devtools' Cleopatra profiler.
 
Extra warnings include:
* assignment to undeclared variable x
* reference to undefined property x.y
* redeclaration of var x
* applying the 'delete' operator to an unqualified name is deprecated
* octal literals and octal escape sequences are deprecated
* test for equality (==) mistyped as assignment (=)?
* function x does not always return a value
* useless expression
* variable x redeclares argument

Comment 15

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

(In reply to Chris Peterson (:cpeterson) from comment #14)
> Extra warnings mode reports many of the same issues as ES5 strict mode

Is this true? My understanding is that they were mostly unrelated. I'm generally skeptical about this pref because I think its signal/noise ratio is rather low.

Comment 16

[Kris Maglione [:kmag]]

I believe that the only overlap is in assignments to undeclared variables, use of octal literals, and calling 'delete' on unqualified names. And I agree that the signal to noise ratio is far too low even with just builtin code (and especially with things like the Orion editor in Scratchpad). With add-ons, it's generally unsupportable.

Comment 17

[Chris Peterson [:cpeterson]]

Comment on attachment 8352615 [details] [diff] [review]
extra-warnings.patch

Removing f? because the conclusion is that SpiderMonkey's extra warnings are too noisy. Note, however, that extra warnings *are* already enabled by default in Firefox debug builds.