788419 - crash in js::DebugScopeProxy::handleUnaliasedAccess

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This bug was filed from the Socorro interface and is 
report bp-2c8c7d52-2297-4a48-9bd6-f0c752120905 .
============================================================= 

According to Firebug automated test suite, this crash appeared around 3rd of September.

STR:

1) Install Firebug from here:
https://getfirebug.com/releases/firebug/1.11/firebug-1.11.0a2.xpi
2) Install FBTest extension from here:
https://getfirebug.com/releases/fbtest/1.11/fbTest-1.11b1.xpi
3) Open Firebug
4) open FBTest, Firebug (icon) menu -> Open Test Console
5) Press 'Run All' button at the top of the test console window

Doesn't happen all the time

Honza

Comment 1

[Sebastian Zartner [:sebo]]

I also already experienced this today:

https://crash-stats.mozilla.com/report/index/bp-24040e56-d0aa-4a4b-a010-131962120905

Sebastian

Comment 2

[Simon Lindholm]

More precise STR:
 - Install Firebug, and open https://getfirebug.com/tests/head/script/callstack/4845/issue4845.html
 - Reload with script panel enabled - execution should stop at a debugger; statement in secondLevelFunction()
 - Press Shift-F8 (= re-run) -> sometimes it crashes

https://crash-stats.mozilla.com/report/index/4bd04342-cd1e-48fd-aa69-3a2c82120905

I tried a debug build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/1346844012/ but didn't see any assertion failures. Here's a (symbol-less) stack from that:
https://crash-stats.mozilla.com/report/index/55bb26a0-d404-4623-945d-6ea0e2120905

Comment 3

[Luke Wagner [:luke]]

Attachment: fix and test

Oh gosh, that loop is flagrantly wrong.  Pretty hard to exercise since has() shields handleUnaliasedAccess from random name lookups; you have to add a new property with frame.eval and then access it.

Comment 4

[Jim Blandy :jimb]

Comment on attachment 658557 [details] [diff] [review]
fix and test

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

The fix to the loop looks obviously correct, but I wanted to understand why the test case trips the bug.

I take it the situation here is:
- f is not heavyweight, and has no locals, so its bindings list is just the empty shape.
- When we call GetDebugScope we create an ersatz CallObject for it.
- When we evaluate the 'var blah=43':
  - The 'has' method's loop over the bindings is properly written, so defineProperty creates 
    a property on the CallObject that has no counterpart in the script's
    Bindings list.
  - When we store the value from the 'var' statement's initializer, the 'set' method
    gives handleUnaliasedAccess a shot first. The bogus loop then trips over the empty
    bindings list, instead of simply failing, returning false, and letting 'set' write
    the value to the ersatz CallObject's property.

Is that right?

Comment 5

[Jim Blandy :jimb]

... and the 'blah = 44' test is just there to ensure that ordinary stores, not associated with any definition, also work.

Comment 6

[Luke Wagner [:luke]]

You are correct sir!

Comment 7

[Luke Wagner [:luke]]

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

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 658557 [details] [diff] [review]
fix and test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 780647
User impact if declined: debugger crashes
Testing completed (on m-c, etc.): m-c

Comment 9

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6bc6c8a55543

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for quick fix!

What is the easiest way to find out whether the path is already part of Nightly?

honza

Comment 12

[Alex Keybl [:akeybl]]

(In reply to Luke Wagner [:luke] from comment #8)
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 780647
> User impact if declined: debugger crashes
> Testing completed (on m-c, etc.): m-c

Luke - what's the risk profile here? I ask because if anything but low, we may want to point QA at this bug specifically for exploratory regression testing, or discuss alternatives.

Comment 13

[Jim Blandy :jimb]

(In reply to Alex Keybl [:akeybl] from comment #12)
> Luke - what's the risk profile here? I ask because if anything but low, we
> may want to point QA at this bug specifically for exploratory regression
> testing, or discuss alternatives.

I would say the risk profile for this patch is low: the loop simply fails to check its conditions in the right order, and fails when there are no iterations to perform. The fix is the obvious rearrangement.

It's the conditions for running into the bug, as given in the test case, that are subtle.

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 658557 [details] [diff] [review]
fix and test

[Triage Comment]
Thanks Jim. Given where we are in the cycle, and the low risk profile, we'll take this in support of Firebug workflows working correctly.