The default bug view has changed. See this FAQ.

crash in js::DebugScopeProxy::handleUnaliasedAccess

RESOLVED FIXED in Firefox 17

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Honza, Assigned: luke)

Tracking

({crash})

18 Branch
mozilla18
x86
Windows NT
crash
Points:
---

Firefox Tracking Flags

(firefox17+ fixed)

Details

(Whiteboard: [firebug-p1], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Whiteboard: [firebug-p1]
I also already experienced this today:

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

Sebastian

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #658557 - Flags: review?(jimb)
(Assignee)

Updated

5 years ago
status-firefox17: --- → affected
tracking-firefox17: --- → ?

Comment 4

5 years ago
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?
Attachment #658557 - Flags: review?(jimb) → review+

Comment 5

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

Comment 6

5 years ago
You are correct sir!
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ba5257ab22
(Assignee)

Comment 8

5 years ago
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
Attachment #658557 - Flags: approval-mozilla-aurora?

Updated

5 years ago
tracking-firefox17: ? → +
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6bc6c8a55543
status-firefox17: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/f2ba5257ab22
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Reporter)

Comment 11

5 years ago
Thanks for quick fix!

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

honza
(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

5 years ago
(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 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.
Attachment #658557 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.