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
I also already experienced this today: https://crash-stats.mozilla.com/report/index/bp-24040e56-d0aa-4a4b-a010-131962120905 Sebastian
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
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 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?
... and the 'blah = 44' test is just there to ensure that ordinary stores, not associated with any definition, also work.
You are correct sir!
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
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.
(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.