Closed
Bug 524743
Opened 15 years ago
Closed 15 years ago
Shape regeneration still does not touch most empty scopes [@ js_Interpret:4436]
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta3-fixed |
blocking1.9.1 | --- | - |
People
(Reporter: jorendorff, Assigned: jorendorff)
References
Details
(Keywords: crash, regression, testcase, Whiteboard: fixed-in-tracemonkey)
Crash Data
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #505932 +++ The patch for bug 505932, as reviewed, says: >+ /* Also regenerate the shapes of empty scopes, in case they are not shared. */ >+ for (JSScope *empty = scope->emptyScope; >+ empty && (empty->flags & JSScope::SHAPE_REGEN) != regenFlag; >+ empty = empty->emptyScope) { >+ empty->shape = js_RegenerateShapeForGC(cx); >+ empty->flags ^= JSScope::SHAPE_REGEN; >+ } One of Brendan's review comments was: >Add a scope->hasRegenFlag(regenFlag) inline helper. Maybe call it >matchRegenFlag? but when I did that, I changed the code from: >+ empty && (empty->flags & JSScope::SHAPE_REGEN) != regenFlag; to: >+ empty && empty->hasRegenFlag(regenFlag); The ! in the original != was lost. The sense of the expression was flipped. Analysis and patch to follow.
Assignee | ||
Comment 2•15 years ago
|
||
The comment in JSRuntime about rt->gcRegenShapesScopeFlag explains: /* * During gc, if rt->gcRegenShapes && * (scope->flags & JSScope::SHAPE_REGEN) == rt->gcRegenShapesScopeFlag, * then the scope's shape has already been regenerated during this GC. * To avoid having to sweep JSScopes, the bit's meaning toggles with each * shape-regenerating GC. * * FIXME Once scopes are GC'd (bug 505004), this will be obsolete. */ uint8 gcRegenShapesScopeFlag; The comment is correct. New scopes are given the current value of this flag. The flag is toggled at the *start* of each shape-regenerating GC. So during "ordinary time", this runtime-wide flag matches the corresponding bit in all existing scopes, new and old. After the toggle, all scopes *do not* match, such that scope->hasRegenFlag(rt->gcRegenShapesScopeFlag) would be false: "no, my shape has not been regenerated yet". Clearly we want to regenerate shapes for scopes that have *not* been regenerated yet, as in the patch. Apologies to brendan and dmandelin for the bug and for not suspecting my own code until prompted. :-P
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 3•15 years ago
|
||
I can't yet reproduce bug 519363's crash signature, but v2 adds an assertion in JSScope::add and a test that reliably flunks the assertion if you revert the fixes in jsscopeinlines.h.
Attachment #408651 -
Attachment is obsolete: true
Attachment #408668 -
Flags: review?(brendan)
Attachment #408651 -
Flags: review?(brendan)
Comment 4•15 years ago
|
||
Comment on attachment 408668 [details] [diff] [review] v2 - add a test, always do shape regeneration if gczeal >= 1. >+ if (rt->shapeGen & SHAPE_OVERFLOW_BIT >+#ifdef JS_GC_ZEAL >+ || rt->gcZeal >= 1 >+#endif >+ ) { Nice, we've wanted testability here, should have thought of gczeal instead of a setShapeGen shell cmd or whatever was proposed the other week... /be
Attachment #408668 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 5•15 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/d54c92b7f76e
Flags: blocking1.9.2?
Whiteboard: fixed-in-tracemonkey
Updated•15 years ago
|
Flags: in-testsuite+
Comment 6•15 years ago
|
||
(In reply to comment #5) > http://hg.mozilla.org/tracemonkey/rev/d54c92b7f76e jorendorrf: This test needs to turn off gczeal before exiting. Otherwise in the browser based jsreftests gczeal(2) will remain in effect for the remaining tests.
Comment 7•15 years ago
|
||
fixed the test. Note I had to disable it in the debug browser due to a hang. Is that a bug? Also note I set gTestfile. This along with the defined setter for gTestfile outputs a "begin test:" line when a test starts. This is very helpful for determining which test last began in the test logs.
Assignee | ||
Comment 9•15 years ago
|
||
(In reply to comment #7) > fixed the test. Note I had to disable it in the debug browser due to a hang. > Is that a bug? Hmm. This test could run GC maybe 500 times. That shouldn't hang the browser, but if the test framework accumulates objects as it runs tests, it might. Does it hang if you run the test all by itself? If so, it's a bug. > Also note I set gTestfile. This along with the defined setter for > gTestfile outputs a "begin test:" line when a test starts. This is very helpful > for determining which test last began in the test logs. Filed bug 525213 about gTestFile.
Comment 10•15 years ago
|
||
Not sure how long to wait, but it sure looks like a hang to me. you can run it via ./firefox-debug/dist/bin/firefox -P test ./js/src/tests/jsreftest.html?test=js1_8_1/regress/regress-524743.js it's cleaner if your profile has js/src/tests/user.js
Comment 12•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d54c92b7f76e
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Updated•15 years ago
|
blocking1.9.1: --- → ?
Comment 13•15 years ago
|
||
There's no reason given for the blocking nom, but I guess I'm assuming from the dependency tree it's meant to be a fix for topcrash bug 519363? But there's patches in that bug too, and we don't know if this patch fixes the topcrashes. And what's with the blocking dependency on the similarly named bug 505932? Is that required/assumed by these patches and therefore also required on the 1.9.1 branch? Clearing 1.9.1 nomination until we get more details
blocking1.9.1: ? → ---
Comment 14•15 years ago
|
||
This patch fixes the crash named in bug 519363. The patches in that bug are not fixes; they are instrumentation that was used as part of debugging this. I believe this bug was marked blocking bug 505932 because it fixes a bug introduced by that bug. (Actually, that is not quite true; it fixes a bug that 505932 tried to fix but didn't. 505932 in turn is marked as blocking that bug. So the usual "if you land 505932 then you should land this" sense applies.)
Comment 15•15 years ago
|
||
(In reply to comment #14) > So the usual "if you land 505932 then you should land this" sense applies. But is the reverse true? If we land this bug, do we need 505932?
blocking1.9.1: --- → ?
Comment 16•15 years ago
|
||
(In reply to comment #15) > (In reply to comment #14) > > So the usual "if you land 505932 then you should land this" sense applies. > > But is the reverse true? If we land this bug, do we need 505932? Yes, this would land as the top of a stack of 5 patches: 3 patches for bug 503080 1 patch for bug 505932 1 patch for bug 524743 (this one)
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #15) > (In reply to comment #14) > > So the usual "if you land 505932 then you should land this" sense applies. > > But is the reverse true? If we land this bug, do we need 505932? Yeah, this patch fixes a bug introduced in bug 505932.
Comment 18•15 years ago
|
||
OK, but then why is this bug nominated as a "blocker" for 1.9.1 if the bug causing the regression hasn't landed on 1.9.1 (or been nominated)? and that one was a regression from bug 503080 which also didn't land on 1.9.1 and isn't nominated. That one affects 1.9.1 (marked "wanted"), but with a 60K patch I'm not sure we really want to take it unless it's fixing a serious problem. I guess bug 503080 is the fix for bug 501986, which is a security bug we do want to block on. Ugh. With that kind of an extended chain I'm not all that confident we can successfully land the patches on old branches and not introduce more regressions :-( Looks like Sam has the same fears (bug 501986 comment 22)
Comment 19•15 years ago
|
||
Frankly, this set of bugs is far too confusing. To fix the js_Interpret topcrash, drivers would like the following: * one new bug filed that's 1.9.1-only * a list of all patches we're taking to fix the js_Interpret topcrash and the bugs they relate to * a list of all other dependent bugs in the dependencies and why we don't need to take them as well * a roll-up patch that has everything we need This bug, in and of itself, is not blocking. Please file a new bug and we'll consider it for blocking 1.9.1.7, but it's likely too late for 1.9.1.6.
blocking1.9.1: ? → -
Comment 20•15 years ago
|
||
(In reply to comment #19) > > This bug, in and of itself, is not blocking. Please file a new bug and we'll > consider it for blocking 1.9.1.7, but it's likely too late for 1.9.1.6. Um no. That is ridiculous.
Comment 21•15 years ago
|
||
I think the explanations spread across a dozen bugs is ridiculous and what I'm asking for isn't at all unreasonable given the state of all these bugs.
Comment 22•15 years ago
|
||
(In reply to comment #21) > I think the explanations spread across a dozen bugs is ridiculous and what I'm > asking for isn't at all unreasonable given the state of all these bugs. We'll make a new bug, and then it will go on 1.9.1.6 if we determine it is reasonable.
Comment 23•15 years ago
|
||
Ah, sure. If the patch is reasonable, we might still include it in 1.9.1.6, but keep in mind that code freeze is this coming Tuesday.
Comment 24•15 years ago
|
||
(In reply to comment #23) > Ah, sure. If the patch is reasonable, we might still include it in 1.9.1.6, but > keep in mind that code freeze is this coming Tuesday. Drivers seem particularly poorly placed to make a determination of the riskiness of the patch. Don't write stuff like that.
Assignee | ||
Comment 25•15 years ago
|
||
(In reply to comment #19) > Frankly, this set of bugs is far too confusing. I appreciate your frankness, but I can't tell if you mean "argh, the world is too messy" or "you guys aren't using Bugzilla properly". Anyway I agree that the bugs are confusing, and the Bugzilla record doesn't help much. But that's how Bugzilla is. It's write-only. It records the history of the mess. It does not support your use case here; the only way to get the big picture is to read all the comments and deliberately cross-reference dates etc. Bug 251556 is another significant source of confusion here, I think (see bug 251556 comment 1 and 2). I think the Bugzilla folks could fix that without much more effort, but I don't think anyone is working on it. Filed bug 526834 for getting root bug 503080 and its hangers-on into 1.9.1.
Comment 27•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/37c064e9ddee For some reason, this was marked 'status1.9.2: beta1-fixed' already, which seems wrong because it wasn't landed to 1.9.2 until just now. I'm not updating the flag now in order not to cause further confusion.
Updated•15 years ago
|
Summary: Shape regeneration still does not touch most empty scopes → Shape regeneration still does not touch most empty scopes [@ js_Interpret:4436]
Updated•13 years ago
|
Crash Signature: [@ js_Interpret:4436]
Updated•13 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•