Last Comment Bug 788419 - crash in js::DebugScopeProxy::handleUnaliasedAccess
: crash in js::DebugScopeProxy::handleUnaliasedAccess
Status: RESOLVED FIXED
[firebug-p1]
: crash
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 18 Branch
: x86 Windows NT
: -- critical (vote)
: mozilla18
Assigned To: Luke Wagner [:luke]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-05 00:14 PDT by Jan Honza Odvarko [:Honza]
Modified: 2012-09-07 06:37 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
fix and test (1.68 KB, patch)
2012-09-05 11:19 PDT, Luke Wagner [:luke]
jimb: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jan Honza Odvarko [:Honza] 2012-09-05 00:14:05 PDT
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] 2012-09-05 03:27:42 PDT
I also already experienced this today:

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

Sebastian
Comment 2 Simon Lindholm 2012-09-05 09:04:31 PDT
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] 2012-09-05 11:19:34 PDT
Created attachment 658557 [details] [diff] [review]
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 2012-09-05 12:35:16 PDT
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 2012-09-05 12:37:00 PDT
... 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] 2012-09-05 13:59:45 PDT
You are correct sir!
Comment 8 Luke Wagner [:luke] 2012-09-05 14:20:38 PDT
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 10 Ryan VanderMeulen [:RyanVM] 2012-09-05 19:37:01 PDT
https://hg.mozilla.org/mozilla-central/rev/f2ba5257ab22
Comment 11 Jan Honza Odvarko [:Honza] 2012-09-06 01:46:22 PDT
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] 2012-09-06 08:07:55 PDT
(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 2012-09-06 11:36:32 PDT
(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] 2012-09-07 06:37:57 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.