Disabling strict warnings as errors in xpcshell

[Eddy Bruel]

Strict warnings as errors has been a continuous source of pain for the
devtools team. Substantial areas of our code area currently not strict
warning free.

The reason this leads to problems is that the way the werror flag is
propagated makes no sense: it is a context wide flag, which individual
scripts inherit from the context in which they are first compiled. To
give you an idea how this leads to problems: werror is enabled by
default for the main context in xpcshell tests, but disabled for the
context used to JSMs. As a result, a single line patch like this:

https://pastebin.mozilla.org/5084279

causes strict warnings as errors to be enabled for the entire debugger
server: the "devtools/server/actors/script" module depends on the
"devtools/server/main" module which is also loaded by dbg-server.jsm in
the line below. But since our CommonJS loader caches modules, the JSM
now gets the cached version of "devtools/server/main" which was loaded
in the main context, instead of the version loaded in the JSM's context,
which is what happened before this patch.

Needless to say, it's often impossible to predict what changes to the
code cause strict warnings as errors to be enabled, and when it does
happen, it's often very hard to ascertain what change caused it.

One could argue that we should simply fix all strict errors as we run
into them, but this is a huge time drain in and of itself. For instance,
it turns out that all the xpcshell tests for the debugger server are
broken in the sense that they pass a string representing an error
message as the last argument to the the testing functions, whereas
xpcshell expects this to be a stack object. Because the tests are
broken, if you accidentally turn on strict warnings as errors, this
breaks error reporting in xpcshell itself, because it's trying to use a
string as a stack object.

I would like to propose that we get rid of strict warnings as errors for
xpcshell tests. To quote Nick Fitzgerald: "The
strict-warnings-as-an-error feels like having some arbitrary linter
forced upon our development that isn't very useful.". Achieving strict
warning free code is not a goal for us, and I doubt it is for anyone else.

[Fitzgerald, Nick]

The most annoying thing for me (other than seemingly arbitrary changes
causing whole suites to fail despite correct functionality) is that it
breaks duck typing and causes code like:

if (obj.quacks) {
...
}

to fail tests. So in these cases, we are forced to write code something
like:

if (obj.quacks || false) {
...
}

to sidestep warnings. It doesn't make the code any more readable or
safe, its just an arbitrary change to please an arbitrary linter.

There is real pain we are feeling while dealing with these
strict-warnings-as-errors, and only hand wavy benefit. As Eddy said,
strict warning free code is not a goal for devtools, it is just
something we are forced to do instead of work on whatever we are
actually trying to do!

Nick

[Chris Peterson]

On 5/6/14, 1:49 PM, Eddy Bruel wrote:
> Strict warnings as errors has been a continuous source of pain for the
> devtools team. Substantial areas of our code area currently not strict
> warning free.
>
> The reason this leads to problems is that the way the werror flag is
> propagated makes no sense: it is a context wide flag, which individual
> scripts inherit from the context in which they are first compiled. To
> give you an idea how this leads to problems: werror is enabled by
> default for the main context in xpcshell tests, but disabled for the
> context used to JSMs. As a result, a single line patch like this:

btw, I believe the JS team now uses the term "extra warnings" to
differentiate SpiderMonkey's non-standard "strict warnings"
(javascript.options.strict pref) from ES5's "strict mode".

Does anyone actually want these strict/extra warnings? Can we just
remove this misfeature from SpiderMonkey?

These strict/extra warnings are also enabled in Firefox's debug builds
(javascript.options.strict.debug pref), which can cause Firefox features
to misbehave differently in debug and release builds. Besides the
`obj.quacks || false` problem, it can also report false positive
warnings about some functions' unreachable code paths not returning a value.


chris

[Bill McCloskey]

----- Original Message -----
> From: "Chris Peterson" <cpet...@mozilla.com>
> To: dev-pl...@lists.mozilla.org
> Sent: Tuesday, May 6, 2014 3:11:40 PM
> Subject: Re: Disabling strict warnings as errors in xpcshell
>
> btw, I believe the JS team now uses the term "extra warnings" to
> differentiate SpiderMonkey's non-standard "strict warnings"
> (javascript.options.strict pref) from ES5's "strict mode".
>
> Does anyone actually want these strict/extra warnings? Can we just
> remove this misfeature from SpiderMonkey?

I find this feature extremely useful. When chrome code contains a typo, without extraWarnings we often just produce no error at all. The most common case is that Firefox opens to a blank window (i.e., no content is drawn) and no output. To solve these bugs, I've had to bisect my own code to find the typo. When it turns out to be something as simple as a typo, it's extremely frustrating. The extraWarnings option makes it very easy to quickly identify these sorts of bugs.

In addition, I think the |if (obj.quacks)| example may be overly simplified. The JS engine is smart enough to recognize this pattern and not warn about it. As far as I know, there's no need to change it to |if (obj.quacks || false)|. The time you typically need to pull tricks like that is when passing optional properties to function, as in |func(foo, bar, obj.quacks)| when quacks may not be defined. This seems like bad practice to me though. It's not at all clear to the reader that you might not expect the property to be defined.

> These strict/extra warnings are also enabled in Firefox's debug builds
> (javascript.options.strict.debug pref), which can cause Firefox features
> to misbehave differently in debug and release builds.

As far as I know, the warnings never change behavior. They just cause us to print warnings.

> Besides the
> `obj.quacks || false` problem, it can also report false positive
> warnings about some functions' unreachable code paths not returning a value.

I agree that the unreachable code warning is pretty useless. I'd be fine removing it. The undefined property one is the only useful one that I know of.

-Bill

[Fitzgerald, Nick]

On 5/6/14, 3:29 PM, Bill McCloskey wrote:
> ----- Original Message -----
>> From: "Chris Peterson" <cpet...@mozilla.com>
>> To: dev-pl...@lists.mozilla.org
>> Sent: Tuesday, May 6, 2014 3:11:40 PM
>> Subject: Re: Disabling strict warnings as errors in xpcshell
>>

>> btw, I believe the JS team now uses the term "extra warnings" to
>> differentiate SpiderMonkey's non-standard "strict warnings"
>> (javascript.options.strict pref) from ES5's "strict mode".
>>
>> Does anyone actually want these strict/extra warnings? Can we just
>> remove this misfeature from SpiderMonkey?

> I find this feature extremely useful. When chrome code contains a typo, without extraWarnings we often just produce no error at all. The most common case is that Firefox opens to a blank window (i.e., no content is drawn) and no output. To solve these bugs, I've had to bisect my own code to find the typo. When it turns out to be something as simple as a typo, it's extremely frustrating. The extraWarnings option makes it very easy to quickly identify these sorts of bugs.

Perhaps a more extensible solution would be the option to turn the extra
warnings on/off for different test suites. Could we just flip the
javascript.options.strict.debug pref off in our xpcshell head.js to
disable the extra warnings for our test suite?

>> These strict/extra warnings are also enabled in Firefox's debug builds
>> (javascript.options.strict.debug pref), which can cause Firefox features
>> to misbehave differently in debug and release builds.

> As far as I know, the warnings never change behavior. They just cause us to print warnings.

At least in the devtools xpcshell tests, we listen for console error
messages and fail tests when we receive them. We do this because the
xpcshell tests have historically swallowed up a lot of our errors
resulting in enigmatic timeouts with no clues as to why they timed out.
This is very valuable to us, but unfortunately the extra warnings are
greatly reducing its value.

[Jonas Sicking]

On Tue, May 6, 2014 at 4:13 PM, Fitzgerald, Nick
<nfitz...@mozilla.com> wrote:
> Perhaps a more extensible solution would be the option to turn the extra
> warnings on/off for different test suites.

Are these warnings ever useful enough that this is worth it?

/ Jonas

[Fitzgerald, Nick]

On 5/6/14, 5:31 PM, Jonas Sicking wrote:
> Are these warnings ever useful enough that this is worth it?

Personally, I think not. Bill seemed to really like them. *shrug*

This was why I thought it might be easier to just make it optional.

[Chris Peterson]

IIUC, Eddy's original concern was not about strict warnings, but that
they were treated as errors. The simple solution is to remove the
warnings-as-errors option. :)


chris

[Gavin Sharp]

On Tue, May 6, 2014 at 1:49 PM, Eddy Bruel <ejpb...@mozilla.com> wrote:
> I would like to propose that we get rid of strict warnings as errors for
> xpcshell tests. To quote Nick Fitzgerald: "The strict-warnings-as-an-error
> feels like having some arbitrary linter forced upon our development that
> isn't very useful.". Achieving strict warning free code is not a goal for
> us, and I doubt it is for anyone else.

What does "get rid of strict warnings as errors for xpcshell tests"
mean in practice?

I don't understand how you're getting into the situation of
"accidentally turn[ing] on strict warnings as errors".

Gavin

[Fitzgerald, Nick]

On 5/7/14, 4:21 PM, Gavin Sharp wrote:
> What does "get rid of strict warnings as errors for xpcshell tests"
> mean in practice?

It means that our non-standard spidermonkey "strict mode" (not the
actual strict mode) console warnings would continue to simply be console
warning messages rather than console error messages in xpcshell tests.

> I don't understand how you're getting into the situation of
> "accidentally turn[ing] on strict warnings as errors".

Eddy can explain this better than me because he's been deep in these
trenches the last couple weeks, but I'll give it a shot.

When xpcshell tests are run, they flip a bit on the initial JSContext
that's off by default that tells spidermonkey "make the strict warning
messages into error messages". Depending on how you load JS code, you
might share the JSContext or you might not; for example, loadSubScript
shares the JSContext, while Cu.import doesn't. Eddy has been making
changes to the debugger server so that it will run in workers so we can
debug workers. He has been replacing Cu.import calls with calls to a
module loader that uses loadSubScript underneath the hood. So now code
that used to be evaluated with this bit flipped off (because it is off
by default and it was getting its own JSContext) is being evaluated with
the bit on (because it is inheriting the JSContext from the xpcshell
test). The result is error messages which cause devtools tests to fail.

[Gavin Sharp]

> When xpcshell tests are run, they flip a bit on the initial JSContext that's
> off by default that tells spidermonkey "make the strict warning messages
> into error messages".

Do you have a pointer to where this happens? I've never heard of this,
and couldn't find it MXRing.

Gavin

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Gavin Sharp]

To elaborate:
- Bug 524781 is still open
- I don't see any reference to -werror or -S in runxpcshelltests.py

Gavin

[Eddy Bruel]

To be clear, I don't actually know where the werror flag for xpcshell
tests is set.

What I've observed is:
- If we Cu.import a JSM that requires module X, and then also require
module X from head_dbg.js after that, all is well.
- If we require module X from head_dbg.js first, and then also
require module X from the JSM after that, we get strict errors.

What I've gathered from reading the code is:
- Each JSContext has a werror flag, which is set to false by default.
- Each JSScript inherits the werror flag from the JSContext in which
it is compiled.
- loadSubScript uses the JSContext of its caller.
- Cu.import loads JSM's in a separate JSContext, which doesn't have
the flag set.
- Our CommonJS loader uses loadSubScript under the hood, and caches
modules.

The conclusion I drew from that is that apparently, the werror flag is
set for the context in which head_dbg.js is compiled: if the module gets
loaded from that context first, it inherits the werror flag, causing
strict warnings to be turned into errors. Conversely, if the JSM gets to
load the module first, it gets loaded from the JSM's context, which
doesn't have the flag set, and everything is fine.

This is the best explanation I could come up with based on the behavior
I've observed. It could be that I missed something, and the werror flag
isn't actually set for xpcshell tests, but if that's the case, they are
still coming from *somewhere*, and this only strenghtens my argument
that it's extremely confusing what causes them to be enabled. So my main
point remains, and this is still an issue for us.

I think what we should do is confirm that strict warnings as errors is
actually turned on for xpcshell, and if so, where this happens.

CC'ing bholley, because he probably knows where to look.

[Gavin Sharp]

> I think what we should do is confirm that strict warnings as errors is
> actually turned on for xpcshell, and if so, where this happens.

Indeed. Can you set a breakpoint in toggleWerror and see what trips it?

Gavin