Re: We need better canaries for JS code

[ISHIKAWA,chiaki]

I am for stricter checks and crashing the test when some of the errors 
discussed here are detected.
I have been puzzled when some attributes are undefined as such when I 
ran tests for Thunderbird in the past.

The bugs are found non-Thunderbird-specific code often times.

Crashing the tests would be the only way to force the responsible party 
to fix them.


Also, the mechanism for automatic dumping the stack during testing when 
such errors are encountered will be appreciated. Running gdb under linux 
and do some python magic to look at VM stack is not easy.


However, sending the stack from the user may have a privacy concern, and 
should be evaluated very carefully.
(I mean, VM stack may show the URL which the user was visiting at the 
time exactly, etc., or even some input.)



n 2017/10/18 18:45, Gregory Szorc wrote:

On Wed, Oct 18, 2017 at 10:28 AM, David Teller  wrote:


 Hi everyone,

   Yesterday, Nightly was broken on Linux and MacOS because of a typo in
JS code [1]. If I understand correctly, this triggered the usual
"undefined is not a function", which was

1/ uncaught during testing, as these things often are;
2/ basically impossible to diagnose in the wild, because there was no
error message of any kind.

I remember that we had bugs of this kind lurking for years in our
codebase, in code that was triggered daily and that everybody believed
to be tested.

I'd like to think that there is a better way to handle these bugs,
without waiting for them to explode in our user's face. Opening this
thread to see if we can find a way to somehow "solve" these bugs, either
by making them impossible, or by making them much easier to solve.

I have one proposal. Could we change the behavior of the JS VM as follows?

- The changes affect only Nightly.
- The changes affect only mozilla chrome code (including system add-ons
but not user add-ons or test code).
- Any programmer error (e.g. SyntaxError) causes a crash a crash that
displays (and attaches to the CrashReporter) both the JS stack in and
the native stack.
- Any SyntaxError is a programmer error.
- Any TypeError is a programmer error.

I expect that this will find a number of lurking errorsy, so we may want
to migrate code progressively, using a directive, say "use strict
moz-platform" and static analysis to help encourage using this directive.

What do you think?



I agree that errors like this should have better visibility in order to
help catch bugs.

I'm not sure changing behavior of the JS VM is the proper layer to
accomplish this. I think reporting messages from the JS console is a better
place to start. We could change the test harnesses to fail tests if certain
errors (like SyntaxError or TypeError) are raised. If there is a "hook" in
the JS VM to catch said errors at error time, we could have the test
harnesses run Firefox in a mode that makes said errors more fatal (even
potentially crashing as you suggest) and/or included additional metadata,
such as stacks.

Another idea would be to require all non-log output in the JS console to be
accounted for. Kinda like reftest's expected assertion count. Assuming most
JS errors/warnings are unwanted, this would allow us to fail all tests
reporting JS errors/warnings while allowing wanted/known failures to not
fail the test. A problem though is background services "randomly" injecting
their output during test execution depending on non-deterministic timing.
It could be difficult to roll this out in practice. But it feels like we
should be able to filter out messages or stacks accordingly.

But why stop there? Shouldn't Firefox installs in the wild report JS errors
and warnings in Firefox code back to Mozilla (like we do crashes)? I know
this has been discussed. I'm not sure what we're doing/planning about it
though.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform




___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Matthew N.]

Hi David,

On 2017-10-18 1:28 AM, David Teller wrote:

If I understand correctly, this triggered the usual
"undefined is not a function", which was

1/ uncaught during testing, as these things often are;


That's only because this code doesn't have any tests. That was filed as 
bug 1409913.



2/ basically impossible to diagnose in the wild, because there was no
error message of any kind.


There was an error message[2] which clearly pointed to the problem:
"JavaScript error: resource:///modules/CustomizableUI.jsm, line 461: 
TypeError: placements.spice is not a function"



I remember that we had bugs of this kind lurking for years in our
codebase, in code that was triggered daily and that everybody believed
to be tested.

I'd like to think that there is a better way to handle these bugs,
without waiting for them to explode in our user's face. Opening this
thread to see if we can find a way to somehow "solve" these bugs, either
by making them impossible, or by making them much easier to solve.

I have one proposal. Could we change the behavior of the JS VM as follows?

- The changes affect only Nightly.
- The changes affect only mozilla chrome code (including system add-ons
but not user add-ons or test code).
- Any programmer error (e.g. SyntaxError) causes a crash a crash that
displays (and attaches to the CrashReporter) both the JS stack in and
the native stack.
- Any SyntaxError is a programmer error.
- Any TypeError is a programmer error.

I expect that this will find a number of lurking errorsy, so we may want
to migrate code progressively, using a directive, say "use strict
moz-platform" and static analysis to help encourage using this directive.

What do you think?


I'm not really sure that changing the failure mode from a totally broken 
Firefox window to a crash really helps much. Both would have only 
happened once the builds were in the hands of users. The root cause of 
the issue was that there were no automated tests for the relevant code 
and from my understanding we don't need JS VM changes to address that.


I do agree with other messages in the thread about making our test 
harnesses stricter regarding uncaught errors but without this code being 
tested that wouldn't have helped here.



Cheers,
  David



[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1407351#c28


[2] https://bug1409395.bmoattachments.org/attachment.cgi?id=8919310
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Ted Mielczarek]

On Thu, Oct 19, 2017, at 03:19 PM, Mark Banner wrote:
> The only thing that might help (that has been discussed) is something 
> along the lines of flow - an analyser that could work out that 'spice()' 
> didn't exist, but Dave Townsend mentioned these don't seem to be viable 
> for us.
> 
> Therefore I think the migration code should be having automated tests - 
> it is the only way we can reliably detect these sort of failures at the 
> moment.

That's fair. Note that we do have active code coverage results now,
which shows me that the line containing the bug that started this whole
thread still isn't covered by tests:
https://codecov.io/gh/marco-c/gecko-dev/src/master/browser/components/customizableui/CustomizableUI.jsm#L461

-Ted
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Mark Banner]

On 19/10/2017 14:02, Ted Mielczarek wrote:


On Wed, Oct 18, 2017, at 07:51 AM, Mark Banner wrote:

Looping in firefox-dev as well, as I thin this is an important
discussion.

On 18/10/2017 09:28, David Teller wrote:

  Hi everyone,

Yesterday, Nightly was broken on Linux and MacOS because of a typo in
JS code [1]. If I understand correctly, this triggered the usual
"undefined is not a function", which was

1/ uncaught during testing, as these things often are;

Part of the reason it was uncaught, is that there's no automated test
coverage for that bit of code. It is a migration from one version to the
other, but unless there is an explicit test (and I don't see one) that
line won't be hit.

Given this bit, would any of the suggestions in this thread actually
help? If we're not exercising this code in automated tests, would "fail
tests on uncaught exceptions" make any difference?
The only thing that might help (that has been discussed) is something 
along the lines of flow - an analyser that could work out that 'spice()' 
didn't exist, but Dave Townsend mentioned these don't seem to be viable 
for us.


Therefore I think the migration code should be having automated tests - 
it is the only way we can reliably detect these sort of failures at the 
moment.


Mark.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Dan Mosedale]

Dave, how would you feel about deciding on one of those and allowing
modules to opt-in to using them, perhaps just as an experiment.  Presumably
most existing modules wouldn't, but new ones being written might.

Dan

2017-10-18 9:06 GMT-07:00 Dave Townsend :

> On Wed, Oct 18, 2017 at 4:51 AM Mark Banner  wrote:
>
>> I remember that we had bugs of this kind lurking for years in our
>> codebase, in code that was triggered daily and that everybody believed
>> to be tested.
>>
>> I'd like to think that there is a better way to handle these bugs,
>> without waiting for them to explode in our user's face. Opening this
>> thread to see if we can find a way to somehow "solve" these bugs, either
>> by making them impossible, or by making them much easier to solve.
>>
>> ESLint has caught some bugs - mainly undefined and unused related issues,
>> and is spread through most of the production javascript code. Unfortunately
>> it isn't able to catch this class of error. For that, we'd need something
>> like Flow. Last time I looked at it, it didn't feel like a good fit for us,
>> although I didn't go too deep, and I think there may have been other people
>> that were looking at it.
>>
>
> As a datapoint, I've looked at both Flow and TypeScript. Both are good
> tools that work well if you're writing code from scratch with them but for
> existing code they flag many many pre-existing problems, the vast majority
> of which aren't really problems just cases where the tools can't infer what
> is going on without adding type info to the script. I came to the
> conclusion that it would be too much work to use either in our main
> codebase.
>
>
> ___
> firefox-dev mailing list
> firefox-...@mozilla.org
> https://mail.mozilla.org/listinfo/firefox-dev
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Dan Mosedale]

Could we do this on a per-module opt-in basis to allow for gradual
migration?  That is to say, assuming there's enough information in the
stack to tell where it was thrown from (I'm guessing that's the case most
of the time), by default, ignore these errors unless they're from code in
an opted-in module?

Dan

2017-10-18 11:51 GMT-07:00 Ryan VanderMeulen :

> Note that we also have bug 920191 on file for making JS exceptions fatal
> in the harness. One of the big blockers to that has always been cleaning up
> the existing set of problems, but maybe it would be helpful for some of
> these issues if/when we could drive that into happening.
>
> -Ryan
>
>
> On Wed, Oct 18, 2017 at 12:20 PM, J. Ryan Stinnett 
> wrote:
>
>> This may not be what you want here, but just so you are aware of the
>> option...
>>
>> You can use `Services.console.registerListener`[1] in a test harness,
>> etc. to hear messages logged to the console. The harness could count
>> messages of a certain type (like script errors) and fail the test if
>> there's an unexpected count (similar to assertion handling in some
>> harnesses).
>>
>> Some test harnesses already register console listeners, but many of them
>> just echo the messages to the test log without examining them further.
>>
>> For JS errors logged to the console, you can QI them to nsIScriptError
>> and examine more details like line, column, stack, etc.
>>
>> Anyway, might not cover all the cases envisioned here, but it's one tool
>> available to us.
>>
>> [1]: http://searchfox.org/mozilla-central/search?q=console.regist
>> erListener=true=false=
>>
>> - Ryan
>>
>> On Wed, Oct 18, 2017 at 6:51 AM, Mark Banner  wrote:
>>
>>> Looping in firefox-dev as well, as I thin this is an important
>>> discussion.
>>>
>>> On 18/10/2017 09:28, David Teller wrote:
>>>
>>> Hi everyone,
>>>
>>>   Yesterday, Nightly was broken on Linux and MacOS because of a typo in
>>> JS code [1]. If I understand correctly, this triggered the usual
>>> "undefined is not a function", which was
>>>
>>> 1/ uncaught during testing, as these things often are;
>>>
>>> Part of the reason it was uncaught, is that there's no automated test
>>> coverage for that bit of code. It is a migration from one version to the
>>> other, but unless there is an explicit test (and I don't see one) that line
>>> won't be hit.
>>>
>>> This seems to be a common theme with various migration routes in the
>>> code base, when I've asked about ensuring more automated testing there
>>> before I feel like I've got shrugs.
>>>
>>> Admittedly, a lot of it is simple code, and should be correct, but as
>>> we've seen, mistakes can be made. Should we now be changing our policy?
>>>
>>> At a minimum, I wonder if we could write some sort of simple test for
>>> CustomizableUI that would go through most routes (with minimal updates for
>>> each change).
>>>
>>> 2/ basically impossible to diagnose in the wild, because there was no
>>> error message of any kind.
>>>
>>> I did an experiment, and the only way I got an error out was to have
>>> "javascript.options.strict" on and "browser.dom.window.dump.enabled".
>>> Unfortunately the breakage was such that the browser console couldn't be
>>> opened, so you'd just having strict on wasn't enough.
>>>
>>> I remember that we had bugs of this kind lurking for years in our
>>> codebase, in code that was triggered daily and that everybody believed
>>> to be tested.
>>>
>>> I'd like to think that there is a better way to handle these bugs,
>>> without waiting for them to explode in our user's face. Opening this
>>> thread to see if we can find a way to somehow "solve" these bugs, either
>>> by making them impossible, or by making them much easier to solve.
>>>
>>> ESLint has caught some bugs - mainly undefined and unused related
>>> issues, and is spread through most of the production javascript code.
>>> Unfortunately it isn't able to catch this class of error. For that, we'd
>>> need something like Flow. Last time I looked at it, it didn't feel like a
>>> good fit for us, although I didn't go too deep, and I think there may have
>>> been other people that were looking at it.
>>>
>>> I have one proposal. Could we change the behavior of the JS VM as follows?
>>>
>>> - The changes affect only Nightly.
>>> - The changes affect only mozilla chrome code (including system add-ons
>>> but not user add-ons or test code).
>>> - Any programmer error (e.g. SyntaxError) causes a crash a crash that
>>> displays (and attaches to the CrashReporter) both the JS stack in and
>>> the native stack.
>>> - Any SyntaxError is a programmer error.
>>> - Any TypeError is a programmer error.
>>>
>>> I expect that this will find a number of lurking errorsy, so we may want
>>> to migrate code progressively, using a directive, say "use strict
>>> moz-platform" and static analysis to help encourage using this directive.
>>>
>>> It would definitely be interesting to fail 

Re: We need better canaries for JS code

[Ted Mielczarek]

On Wed, Oct 18, 2017, at 07:51 AM, Mark Banner wrote:
> Looping in firefox-dev as well, as I thin this is an important
> discussion.
> 
> On 18/10/2017 09:28, David Teller wrote:
> >  Hi everyone,
> >
> >Yesterday, Nightly was broken on Linux and MacOS because of a typo in
> > JS code [1]. If I understand correctly, this triggered the usual
> > "undefined is not a function", which was
> >
> > 1/ uncaught during testing, as these things often are;
> Part of the reason it was uncaught, is that there's no automated test 
> coverage for that bit of code. It is a migration from one version to the 
> other, but unless there is an explicit test (and I don't see one) that 
> line won't be hit.

Given this bit, would any of the suggestions in this thread actually
help? If we're not exercising this code in automated tests, would "fail
tests on uncaught exceptions" make any difference?

I'm generally in favor of being stricter about errors in our test suites
etc, but I'm curious about whether we would actually have solved the
problem in question.

-Ted
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[David Teller]

Btw, I believe that there is already support for reporting uncaught
errors  and that it is blocked by the lack of test harness support.

Cheers,
 David

On 18/10/17 19:37, Steve Fink wrote:
> My gut feeling is that you'd only want uncaught errors, and
> AutoJSAPI::ReportException is a better place than setPendingException. I
> don't know how common things like
> 
>   if (eval('nightlyOnlyFeature()')) { ... }
> 
> are, but they certainly seem reasonable. And you'd have to do a bunch of
> work for every one to decide whether the catch was appropriate or not.
> It may be worth doing too, if you could come up with some robust
> whitelisting mechanisms, but at least starting with uncaught exceptions
> seems more fruitful.
> 
> As for the Promise case, I don't know enough to suggest anything, but
> surely there's a way to detect those particular issues separately? Is
> there any way to detect if a finalized Promise swallowed an exception
> without "handling" it in some way or other, even if very heuristically?
> 
> 
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Mark Banner]

On 19/10/2017 03:29, Kris Maglione wrote:


On Wed, Oct 18, 2017 at 07:22:09PM -0700, Daniel Veditz wrote:
On Wed, Oct 18, 2017 at 4:51 AM, Mark Banner  
wrote:



I did an experiment, and the only way I got an error out was to have
"javascript.options.strict" on and



Why isn't it a code-style/review requirement that our own internal JS
include "use strict"? As a quick check I found 659 .jsm files in our 
tree

and only about 500 with "use strict". A quick skim of .js files shows a
similar ratio. It's not terrible (call it a "B" grade?) but we could do
better.


It is, in some areas, depending on their ESLint rules:

http://searchfox.org/mozilla-central/search?q=%22strict%22=true=false=eslint 

Requiring "use strict" globally for ESLint is one of the items I've 
already had a few discussions about. The rough current plan is to get 
ESLint upgraded to version 4 (there's been a few blockers on that), then 
think about enabling more rules. Some of the rules are overlapping with 
what strict mode enforces, so I think we can turn those on earlier, then 
enable use strict slightly later.

And it's automatically enforced everywhere in JSM and JS component code.
To clarify, the "use strict" at the start of those files is just for 
show/to remind developers? Once we get ESLint 4, we can quite easily 
enforce showing "use strict" for all .jsm files.
To date, we've mostly avoided enforcing it for browser window code to 
avoid breaking extensions that used arguments.callee.caller (despite 
my endorsement for breaking those extensions...), but that's not 
really an issue anymore, so we should probably move towards enforcing 
it for all chrome JS.
Side note: fyi, we recently enabled the no-caller rule 
 for ESLint which 
stops this being used. We haven't got 100% ESLint coverage yet, but I 
believe we've covered most production code and it is mainly tests 
remaining (which we're working on 
).


Mark
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[David Teller]

This should be feasible.

Opening bug 1409852 for the low-level support.

On 18/10/17 22:22, Dan Mosedale wrote:
> Could we do this on a per-module opt-in basis to allow for gradual
> migration?  That is to say, assuming there's enough information in the
> stack to tell where it was thrown from (I'm guessing that's the case
> most of the time), by default, ignore these errors unless they're from
> code in an opted-in module?
> 
> Dan
> 
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Kris Maglione]

On Wed, Oct 18, 2017 at 07:22:09PM -0700, Daniel Veditz wrote:

On Wed, Oct 18, 2017 at 4:51 AM, Mark Banner  wrote:


I did an experiment, and the only way I got an error out was to have
"javascript.options.strict" on and



Why isn't it a code-style/review requirement that our own internal JS
include "use strict"? As a quick check I found 659 .jsm files in our tree
and only about 500 with "use strict". A quick skim of .js files shows a
similar ratio. It's not terrible (call it a "B" grade?) but we could do
better.


It is, in some areas, depending on their ESLint rules:

http://searchfox.org/mozilla-central/search?q=%22strict%22=true=false=eslint

And it's automatically enforced everywhere in JSM and JS component code. 
To date, we've mostly avoided enforcing it for browser window code to 
avoid breaking extensions that used arguments.callee.caller (despite my 
endorsement for breaking those extensions...), but that's not really an 
issue anymore, so we should probably move towards enforcing it for all 
chrome JS.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Daniel Veditz]

On Wed, Oct 18, 2017 at 4:51 AM, Mark Banner  wrote:

> I did an experiment, and the only way I got an error out was to have
> "javascript.options.strict" on and
>

​Why isn't it a code-style/review requirement that our own internal JS
include "use strict"? As a quick check I found 659 .jsm files in our tree
and only about 500 with "use strict"​. A quick skim of .js files shows a
similar ratio. It's not terrible (call it a "B" grade?) but we could do
better.

-Dan Veditz
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Dave Townsend]

I'm wary when it comes to code that lives in tree as it's both an added
thing for contributors to learn and an added build step for that code. Also
by doing it only within a module you end up missing out on a bunch of
benefits. For system add-ons that live elsewhere though I think that is up
to the developers of that add-on to decide what they want to do bearing in
mind they'll be adding a build step as part of getting their code into m-c.

On Wed, Oct 18, 2017 at 2:21 PM Dan Mosedale  wrote:

> Dave, how would you feel about deciding on one of those and allowing
> modules to opt-in to using them, perhaps just as an experiment.  Presumably
> most existing modules wouldn't, but new ones being written might.
>
> Dan
> 2017-10-18 9:06 GMT-07:00 Dave Townsend :
>
>> On Wed, Oct 18, 2017 at 4:51 AM Mark Banner  wrote:
>>
>>> I remember that we had bugs of this kind lurking for years in our
>>> codebase, in code that was triggered daily and that everybody believed
>>> to be tested.
>>>
>>> I'd like to think that there is a better way to handle these bugs,
>>> without waiting for them to explode in our user's face. Opening this
>>> thread to see if we can find a way to somehow "solve" these bugs, either
>>> by making them impossible, or by making them much easier to solve.
>>>
>>> ESLint has caught some bugs - mainly undefined and unused related
>>> issues, and is spread through most of the production javascript code.
>>> Unfortunately it isn't able to catch this class of error. For that, we'd
>>> need something like Flow. Last time I looked at it, it didn't feel like a
>>> good fit for us, although I didn't go too deep, and I think there may have
>>> been other people that were looking at it.
>>>
>>
>> As a datapoint, I've looked at both Flow and TypeScript. Both are good
>> tools that work well if you're writing code from scratch with them but for
>> existing code they flag many many pre-existing problems, the vast majority
>> of which aren't really problems just cases where the tools can't infer what
>> is going on without adding type info to the script. I came to the
>> conclusion that it would be too much work to use either in our main
>> codebase.
>>
>>
>> ___
>> firefox-dev mailing list
>> firefox-...@mozilla.org
>> https://mail.mozilla.org/listinfo/firefox-dev
>>
>>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Boris Zbarsky]

On 10/18/17 2:37 PM, Steve Fink wrote:
Is there any way to detect if a finalized Promise swallowed an exception 
without "handling" it in some way or other, even if very heuristically?


We used to have such a way.  It requires doing things during 
finalization, which is not great.


But we can also just do the normal promise detection: a last promise in 
a chain that gets rejected and no one does a catch() before we go to the 
event loop.  We already have that infrastructure in place...


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Ryan VanderMeulen]

Note that we also have bug 920191 on file for making JS exceptions fatal in
the harness. One of the big blockers to that has always been cleaning up
the existing set of problems, but maybe it would be helpful for some of
these issues if/when we could drive that into happening.

-Ryan


On Wed, Oct 18, 2017 at 12:20 PM, J. Ryan Stinnett  wrote:

> This may not be what you want here, but just so you are aware of the
> option...
>
> You can use `Services.console.registerListener`[1] in a test harness,
> etc. to hear messages logged to the console. The harness could count
> messages of a certain type (like script errors) and fail the test if
> there's an unexpected count (similar to assertion handling in some
> harnesses).
>
> Some test harnesses already register console listeners, but many of them
> just echo the messages to the test log without examining them further.
>
> For JS errors logged to the console, you can QI them to nsIScriptError and
> examine more details like line, column, stack, etc.
>
> Anyway, might not cover all the cases envisioned here, but it's one tool
> available to us.
>
> [1]: http://searchfox.org/mozilla-central/search?q=console.
> registerListener=true=false=
>
> - Ryan
>
> On Wed, Oct 18, 2017 at 6:51 AM, Mark Banner  wrote:
>
>> Looping in firefox-dev as well, as I thin this is an important discussion.
>>
>> On 18/10/2017 09:28, David Teller wrote:
>>
>> Hi everyone,
>>
>>   Yesterday, Nightly was broken on Linux and MacOS because of a typo in
>> JS code [1]. If I understand correctly, this triggered the usual
>> "undefined is not a function", which was
>>
>> 1/ uncaught during testing, as these things often are;
>>
>> Part of the reason it was uncaught, is that there's no automated test
>> coverage for that bit of code. It is a migration from one version to the
>> other, but unless there is an explicit test (and I don't see one) that line
>> won't be hit.
>>
>> This seems to be a common theme with various migration routes in the code
>> base, when I've asked about ensuring more automated testing there before I
>> feel like I've got shrugs.
>>
>> Admittedly, a lot of it is simple code, and should be correct, but as
>> we've seen, mistakes can be made. Should we now be changing our policy?
>>
>> At a minimum, I wonder if we could write some sort of simple test for
>> CustomizableUI that would go through most routes (with minimal updates for
>> each change).
>>
>> 2/ basically impossible to diagnose in the wild, because there was no
>> error message of any kind.
>>
>> I did an experiment, and the only way I got an error out was to have
>> "javascript.options.strict" on and "browser.dom.window.dump.enabled".
>> Unfortunately the breakage was such that the browser console couldn't be
>> opened, so you'd just having strict on wasn't enough.
>>
>> I remember that we had bugs of this kind lurking for years in our
>> codebase, in code that was triggered daily and that everybody believed
>> to be tested.
>>
>> I'd like to think that there is a better way to handle these bugs,
>> without waiting for them to explode in our user's face. Opening this
>> thread to see if we can find a way to somehow "solve" these bugs, either
>> by making them impossible, or by making them much easier to solve.
>>
>> ESLint has caught some bugs - mainly undefined and unused related issues,
>> and is spread through most of the production javascript code. Unfortunately
>> it isn't able to catch this class of error. For that, we'd need something
>> like Flow. Last time I looked at it, it didn't feel like a good fit for us,
>> although I didn't go too deep, and I think there may have been other people
>> that were looking at it.
>>
>> I have one proposal. Could we change the behavior of the JS VM as follows?
>>
>> - The changes affect only Nightly.
>> - The changes affect only mozilla chrome code (including system add-ons
>> but not user add-ons or test code).
>> - Any programmer error (e.g. SyntaxError) causes a crash a crash that
>> displays (and attaches to the CrashReporter) both the JS stack in and
>> the native stack.
>> - Any SyntaxError is a programmer error.
>> - Any TypeError is a programmer error.
>>
>> I expect that this will find a number of lurking errorsy, so we may want
>> to migrate code progressively, using a directive, say "use strict
>> moz-platform" and static analysis to help encourage using this directive.
>>
>> It would definitely be interesting to fail tests on some of the strict
>> failures. I was surprised when I recently turned on the pref again to see
>> how many warnings there were.
>>
>> Having looked through a few of those, I've just found at least one issue
>> , though
>> non-critical, and I'm thinking we want to get the no-unused-expressions
>>  rule turned on
>> for ESLint as a result.
>>
>> Mark
>>
>> 

Re: We need better canaries for JS code

[Chris Peterson]

On 2017-10-18 4:51 AM, Mark Banner wrote:

I expect that this will find a number of lurking errorsy, so we may want
to migrate code progressively, using a directive, say "use strict
moz-platform" and static analysis to help encourage using this directive.
It would definitely be interesting to fail tests on some of the strict 
failures. I was surprised when I recently turned on the pref again to 
see how many warnings there were.


Having looked through a few of those, I've just found at least one issue 
, though 
non-critical, and I'm thinking we want to get the no-unused-expressions 
 rule turned on 
for ESLint as a result.


Bug 1394556 just enabled strict mode by default for all JSM components 
in Firefox 57.


Bug 807862 suggested enabling strict mode by default for all builtin 
Firefox JS, but it was resolved incomplete because of concerns about 
add-on compatibility. Nosy add-ons could reach up the stack into Firefox 
JS with code like `arguments.callee.caller.caller.caller.name == 
"sss_duplicateTab"`. Perhaps that is worth revisiting now that we only 
support WebExtensions in 57+.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Steve Fink]

On 10/18/17 10:28 AM, Boris Zbarsky wrote:

On 10/18/17 11:54 AM, David Teller wrote:

Mmmh... I was looking at setPendingException at
http://searchfox.org/mozilla-central/source/js/src/jscntxtinlines.h#435


That will allow you to hook all exceptions, sure, including ones 
caught by try/catch.



My idea would be to do it even on caught errors. It is too easy to catch
errors accidentally, in particular in Promise.


OK.  Then you need to do this in the JS engine, indeed, because catch 
happens entirely inside the JS engine. 


My gut feeling is that you'd only want uncaught errors, and 
AutoJSAPI::ReportException is a better place than setPendingException. I 
don't know how common things like


  if (eval('nightlyOnlyFeature()')) { ... }

are, but they certainly seem reasonable. And you'd have to do a bunch of 
work for every one to decide whether the catch was appropriate or not. 
It may be worth doing too, if you could come up with some robust 
whitelisting mechanisms, but at least starting with uncaught exceptions 
seems more fruitful.


As for the Promise case, I don't know enough to suggest anything, but 
surely there's a way to detect those particular issues separately? Is 
there any way to detect if a finalized Promise swallowed an exception 
without "handling" it in some way or other, even if very heuristically?



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Boris Zbarsky]

On 10/18/17 11:54 AM, David Teller wrote:

Mmmh... I was looking at setPendingException at
http://searchfox.org/mozilla-central/source/js/src/jscntxtinlines.h#435


That will allow you to hook all exceptions, sure, including ones caught 
by try/catch.



My idea would be to do it even on caught errors. It is too easy to catch
errors accidentally, in particular in Promise.


OK.  Then you need to do this in the JS engine, indeed, because catch 
happens entirely inside the JS engine.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[J. Ryan Stinnett]

This may not be what you want here, but just so you are aware of the
option...

You can use `Services.console.registerListener`[1] in a test harness, etc.
to hear messages logged to the console. The harness could count messages of
a certain type (like script errors) and fail the test if there's an
unexpected count (similar to assertion handling in some harnesses).

Some test harnesses already register console listeners, but many of them
just echo the messages to the test log without examining them further.

For JS errors logged to the console, you can QI them to nsIScriptError and
examine more details like line, column, stack, etc.

Anyway, might not cover all the cases envisioned here, but it's one tool
available to us.

[1]:
http://searchfox.org/mozilla-central/search?q=console.registerListener=true=false=

- Ryan

On Wed, Oct 18, 2017 at 6:51 AM, Mark Banner  wrote:

> Looping in firefox-dev as well, as I thin this is an important discussion.
>
> On 18/10/2017 09:28, David Teller wrote:
>
> Hi everyone,
>
>   Yesterday, Nightly was broken on Linux and MacOS because of a typo in
> JS code [1]. If I understand correctly, this triggered the usual
> "undefined is not a function", which was
>
> 1/ uncaught during testing, as these things often are;
>
> Part of the reason it was uncaught, is that there's no automated test
> coverage for that bit of code. It is a migration from one version to the
> other, but unless there is an explicit test (and I don't see one) that line
> won't be hit.
>
> This seems to be a common theme with various migration routes in the code
> base, when I've asked about ensuring more automated testing there before I
> feel like I've got shrugs.
>
> Admittedly, a lot of it is simple code, and should be correct, but as
> we've seen, mistakes can be made. Should we now be changing our policy?
>
> At a minimum, I wonder if we could write some sort of simple test for
> CustomizableUI that would go through most routes (with minimal updates for
> each change).
>
> 2/ basically impossible to diagnose in the wild, because there was no
> error message of any kind.
>
> I did an experiment, and the only way I got an error out was to have
> "javascript.options.strict" on and "browser.dom.window.dump.enabled".
> Unfortunately the breakage was such that the browser console couldn't be
> opened, so you'd just having strict on wasn't enough.
>
> I remember that we had bugs of this kind lurking for years in our
> codebase, in code that was triggered daily and that everybody believed
> to be tested.
>
> I'd like to think that there is a better way to handle these bugs,
> without waiting for them to explode in our user's face. Opening this
> thread to see if we can find a way to somehow "solve" these bugs, either
> by making them impossible, or by making them much easier to solve.
>
> ESLint has caught some bugs - mainly undefined and unused related issues,
> and is spread through most of the production javascript code. Unfortunately
> it isn't able to catch this class of error. For that, we'd need something
> like Flow. Last time I looked at it, it didn't feel like a good fit for us,
> although I didn't go too deep, and I think there may have been other people
> that were looking at it.
>
> I have one proposal. Could we change the behavior of the JS VM as follows?
>
> - The changes affect only Nightly.
> - The changes affect only mozilla chrome code (including system add-ons
> but not user add-ons or test code).
> - Any programmer error (e.g. SyntaxError) causes a crash a crash that
> displays (and attaches to the CrashReporter) both the JS stack in and
> the native stack.
> - Any SyntaxError is a programmer error.
> - Any TypeError is a programmer error.
>
> I expect that this will find a number of lurking errorsy, so we may want
> to migrate code progressively, using a directive, say "use strict
> moz-platform" and static analysis to help encourage using this directive.
>
> It would definitely be interesting to fail tests on some of the strict
> failures. I was surprised when I recently turned on the pref again to see
> how many warnings there were.
>
> Having looked through a few of those, I've just found at least one issue
> , though
> non-critical, and I'm thinking we want to get the no-unused-expressions
>  rule turned on for
> ESLint as a result.
>
> Mark
>
> ___
> firefox-dev mailing list
> firefox-...@mozilla.org
> https://mail.mozilla.org/listinfo/firefox-dev
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Dave Townsend]

On Wed, Oct 18, 2017 at 4:51 AM Mark Banner  wrote:

> I remember that we had bugs of this kind lurking for years in our
> codebase, in code that was triggered daily and that everybody believed
> to be tested.
>
> I'd like to think that there is a better way to handle these bugs,
> without waiting for them to explode in our user's face. Opening this
> thread to see if we can find a way to somehow "solve" these bugs, either
> by making them impossible, or by making them much easier to solve.
>
> ESLint has caught some bugs - mainly undefined and unused related issues,
> and is spread through most of the production javascript code. Unfortunately
> it isn't able to catch this class of error. For that, we'd need something
> like Flow. Last time I looked at it, it didn't feel like a good fit for us,
> although I didn't go too deep, and I think there may have been other people
> that were looking at it.
>

As a datapoint, I've looked at both Flow and TypeScript. Both are good
tools that work well if you're writing code from scratch with them but for
existing code they flag many many pre-existing problems, the vast majority
of which aren't really problems just cases where the tools can't infer what
is going on without adding type info to the script. I came to the
conclusion that it would be too much work to use either in our main
codebase.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[David Teller]

On 18/10/17 14:16, Boris Zbarsky wrote:
> On 10/18/17 4:28 AM, David Teller wrote:
>> 2/ basically impossible to diagnose in the wild, because there was no
>> error message of any kind.
> 
> That's odd.  Was the exception caught or something?  If not, it should
> have shown up in the browser console, at least

In this case, the browser console couldn't be opened. Also, as suggested
by gps, we can probably reuse the same (kind of) mechanism to report
stacks of programming errors in the wild.

> 
>> I have one proposal. Could we change the behavior of the JS VM as
>> follows?
> 
> Fwiw, the JS VM doesn't really do exception handling anymore; we handle
> all that in dom/xpconnect code.

Mmmh... I was looking at setPendingException at
http://searchfox.org/mozilla-central/source/js/src/jscntxtinlines.h#435
. Can you think of a better place to handle this?

>> - The changes affect only Nightly.
>> - The changes affect only mozilla chrome code (including system add-ons
>> but not user add-ons or test code).
> 
> What about test chrome code?  The browser and chrome mochitests are
> pretty hard to tell apart from "normal" chrome code...

Good question. I'm not sure yet. I actually don't know how the tests are
loaded, but I hope that there is a way. Also, we need to test: it is
possible that the code of tests might not be a (big) problem.

>> - Any programmer error (e.g. SyntaxError) causes a crash a crash that
>> displays (and attaches to the CrashReporter) both the JS stack in and
>> the native stack.
> 
> We would have to be a little careful to only include the chrome frames
> in the JS stack.
> 
> But the more important question is this: should this only apply to
> _uncaught_ errors, or also to ones inside try/catch?  Doing the former
> is pretty straightforward, actually.  Just hook into
> AutoJSAPI::ReportException and have it do whatever work you want.  It
> already has a concept of "chrome" (though it may not match the
> definition above; it basically goes by "system principal or not?") and
> should be the bottleneck for all uncaught exceptions, except:
> 
> * Toplevel evaluation errors (including syntax errors) in worker scripts.
> * "uncaught" promise rejections of various sorts
> 
> Doing this might be a good idea.  It's _definitely_ a good experiment...

My idea would be to do it even on caught errors. It is too easy to catch
errors accidentally, in particular in Promise.

Cheers,
 David
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Boris Zbarsky]

On 10/18/17 4:28 AM, David Teller wrote:

2/ basically impossible to diagnose in the wild, because there was no
error message of any kind.


That's odd.  Was the exception caught or something?  If not, it should 
have shown up in the browser console, at least



I have one proposal. Could we change the behavior of the JS VM as follows?


Fwiw, the JS VM doesn't really do exception handling anymore; we handle 
all that in dom/xpconnect code.



- The changes affect only Nightly.
- The changes affect only mozilla chrome code (including system add-ons
but not user add-ons or test code).


What about test chrome code?  The browser and chrome mochitests are 
pretty hard to tell apart from "normal" chrome code...



- Any programmer error (e.g. SyntaxError) causes a crash a crash that
displays (and attaches to the CrashReporter) both the JS stack in and
the native stack.


We would have to be a little careful to only include the chrome frames 
in the JS stack.


But the more important question is this: should this only apply to 
_uncaught_ errors, or also to ones inside try/catch?  Doing the former 
is pretty straightforward, actually.  Just hook into 
AutoJSAPI::ReportException and have it do whatever work you want.  It 
already has a concept of "chrome" (though it may not match the 
definition above; it basically goes by "system principal or not?") and 
should be the bottleneck for all uncaught exceptions, except:


* Toplevel evaluation errors (including syntax errors) in worker scripts.
* "uncaught" promise rejections of various sorts

Doing this might be a good idea.  It's _definitely_ a good experiment...

-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Mark Banner]

Looping in firefox-dev as well, as I thin this is an important discussion.

On 18/10/2017 09:28, David Teller wrote:

 Hi everyone,

   Yesterday, Nightly was broken on Linux and MacOS because of a typo in
JS code [1]. If I understand correctly, this triggered the usual
"undefined is not a function", which was

1/ uncaught during testing, as these things often are;
Part of the reason it was uncaught, is that there's no automated test 
coverage for that bit of code. It is a migration from one version to the 
other, but unless there is an explicit test (and I don't see one) that 
line won't be hit.


This seems to be a common theme with various migration routes in the 
code base, when I've asked about ensuring more automated testing there 
before I feel like I've got shrugs.


Admittedly, a lot of it is simple code, and should be correct, but as 
we've seen, mistakes can be made. Should we now be changing our policy?


At a minimum, I wonder if we could write some sort of simple test for 
CustomizableUI that would go through most routes (with minimal updates 
for each change).



2/ basically impossible to diagnose in the wild, because there was no
error message of any kind.
I did an experiment, and the only way I got an error out was to have 
"javascript.options.strict" on and "browser.dom.window.dump.enabled". 
Unfortunately the breakage was such that the browser console couldn't be 
opened, so you'd just having strict on wasn't enough.

I remember that we had bugs of this kind lurking for years in our
codebase, in code that was triggered daily and that everybody believed
to be tested.

I'd like to think that there is a better way to handle these bugs,
without waiting for them to explode in our user's face. Opening this
thread to see if we can find a way to somehow "solve" these bugs, either
by making them impossible, or by making them much easier to solve.
ESLint has caught some bugs - mainly undefined and unused related 
issues, and is spread through most of the production javascript code. 
Unfortunately it isn't able to catch this class of error. For that, we'd 
need something like Flow. Last time I looked at it, it didn't feel like 
a good fit for us, although I didn't go too deep, and I think there may 
have been other people that were looking at it.

I have one proposal. Could we change the behavior of the JS VM as follows?

- The changes affect only Nightly.
- The changes affect only mozilla chrome code (including system add-ons
but not user add-ons or test code).
- Any programmer error (e.g. SyntaxError) causes a crash a crash that
displays (and attaches to the CrashReporter) both the JS stack in and
the native stack.
- Any SyntaxError is a programmer error.
- Any TypeError is a programmer error.

I expect that this will find a number of lurking errorsy, so we may want
to migrate code progressively, using a directive, say "use strict
moz-platform" and static analysis to help encourage using this directive.
It would definitely be interesting to fail tests on some of the strict 
failures. I was surprised when I recently turned on the pref again to 
see how many warnings there were.


Having looked through a few of those, I've just found at least one issue 
, though 
non-critical, and I'm thinking we want to get the no-unused-expressions 
 rule turned on 
for ESLint as a result.


Mark
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[David Teller]

> I'm not sure changing behavior of the JS VM is the proper layer to
> accomplish this. I think reporting messages from the JS console is a
> better place to start. We could change the test harnesses to fail tests
> if certain errors (like SyntaxError or TypeError) are raised. If there
> is a "hook" in the JS VM to catch said errors at error time, we could
> have the test harnesses run Firefox in a mode that makes said errors
> more fatal (even potentially crashing as you suggest) and/or included
> additional metadata, such as stacks.

Ok, I discussed this with jorendorff.

It shouldn't be too hard to add this hook, plus it should have basically
no overhead. The next step would be to register a test harness handler
to crash (or do something else).

This would later open the door to reporting errors (possibly through
crashing) from Nightly, Beta, Release, ...

My main worry, at this stage, is what we encountered when we started
flagging uncaught async errors: some module owners simply never fixed
their errors, so we had to whitelist large swaths of Firefox code,
knowing that it was misbehaving.


Cheers,
 David
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[David Teller]

On 18/10/17 10:45, Gregory Szorc wrote:
> I agree that errors like this should have better visibility in order to
> help catch bugs.
> 
> I'm not sure changing behavior of the JS VM is the proper layer to
> accomplish this. I think reporting messages from the JS console is a
> better place to start. We could change the test harnesses to fail tests
> if certain errors (like SyntaxError or TypeError) are raised. If there
> is a "hook" in the JS VM to catch said errors at error time, we could
> have the test harnesses run Firefox in a mode that makes said errors
> more fatal (even potentially crashing as you suggest) and/or included
> additional metadata, such as stacks.

Works for me. I'd need to check how much performance this would cost.

> Another idea would be to require all non-log output in the JS console to
> be accounted for. Kinda like reftest's expected assertion count.
> Assuming most JS errors/warnings are unwanted, this would allow us to
> fail all tests reporting JS errors/warnings while allowing wanted/known
> failures to not fail the test. A problem though is background services
> "randomly" injecting their output during test execution depending on
> non-deterministic timing. It could be difficult to roll this out in
> practice. But it feels like we should be able to filter out messages or
> stacks accordingly.

This looks like a much larger undertaking.

> But why stop there? Shouldn't Firefox installs in the wild report JS
> errors and warnings in Firefox code back to Mozilla (like we do
> crashes)? I know this has been discussed. I'm not sure what we're
> doing/planning about it though.

I would be for it, but as a followup.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Gregory Szorc]

On Wed, Oct 18, 2017 at 10:28 AM, David Teller  wrote:

> Hi everyone,
>
>   Yesterday, Nightly was broken on Linux and MacOS because of a typo in
> JS code [1]. If I understand correctly, this triggered the usual
> "undefined is not a function", which was
>
> 1/ uncaught during testing, as these things often are;
> 2/ basically impossible to diagnose in the wild, because there was no
> error message of any kind.
>
> I remember that we had bugs of this kind lurking for years in our
> codebase, in code that was triggered daily and that everybody believed
> to be tested.
>
> I'd like to think that there is a better way to handle these bugs,
> without waiting for them to explode in our user's face. Opening this
> thread to see if we can find a way to somehow "solve" these bugs, either
> by making them impossible, or by making them much easier to solve.
>
> I have one proposal. Could we change the behavior of the JS VM as follows?
>
> - The changes affect only Nightly.
> - The changes affect only mozilla chrome code (including system add-ons
> but not user add-ons or test code).
> - Any programmer error (e.g. SyntaxError) causes a crash a crash that
> displays (and attaches to the CrashReporter) both the JS stack in and
> the native stack.
> - Any SyntaxError is a programmer error.
> - Any TypeError is a programmer error.
>
> I expect that this will find a number of lurking errorsy, so we may want
> to migrate code progressively, using a directive, say "use strict
> moz-platform" and static analysis to help encourage using this directive.
>
> What do you think?
>

I agree that errors like this should have better visibility in order to
help catch bugs.

I'm not sure changing behavior of the JS VM is the proper layer to
accomplish this. I think reporting messages from the JS console is a better
place to start. We could change the test harnesses to fail tests if certain
errors (like SyntaxError or TypeError) are raised. If there is a "hook" in
the JS VM to catch said errors at error time, we could have the test
harnesses run Firefox in a mode that makes said errors more fatal (even
potentially crashing as you suggest) and/or included additional metadata,
such as stacks.

Another idea would be to require all non-log output in the JS console to be
accounted for. Kinda like reftest's expected assertion count. Assuming most
JS errors/warnings are unwanted, this would allow us to fail all tests
reporting JS errors/warnings while allowing wanted/known failures to not
fail the test. A problem though is background services "randomly" injecting
their output during test execution depending on non-deterministic timing.
It could be difficult to roll this out in practice. But it feels like we
should be able to filter out messages or stacks accordingly.

But why stop there? Shouldn't Firefox installs in the wild report JS errors
and warnings in Firefox code back to Mozilla (like we do crashes)? I know
this has been discussed. I'm not sure what we're doing/planning about it
though.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: We need better canaries for JS code

[Gabriele Svelto]

On 18/10/2017 10:28, David Teller wrote:
> I have one proposal. Could we change the behavior of the JS VM as follows?
> 
> - The changes affect only Nightly.
> - The changes affect only mozilla chrome code (including system add-ons
> but not user add-ons or test code).
> - Any programmer error (e.g. SyntaxError) causes a crash a crash that
> displays (and attaches to the CrashReporter) both the JS stack in and
> the native stack.
> - Any SyntaxError is a programmer error.
> - Any TypeError is a programmer error.

I'd love to have that, even only on try it would catch a lot of stuff.
BTW the problem of errors in JS code not affecting test results in any
way is very widespread. We have plenty of tests spewing out actual
errors (often only when certain races occur and objects/functions become
undefined) that have no visible impact on the test results themselves
since they're ignored.

 Gabriele



signature.asc
Description: OpenPGP digital signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform