Closed Bug 524743 Opened 11 years ago Closed 11 years ago

Shape regeneration still does not touch most empty scopes [@ js_Interpret:4436]

Categories

(Core :: JavaScript Engine, defect, P1)

x86
macOS
defect

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)

+++ 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.
BTW, this might very well be the cause of topcrash bug 519363.
Attached patch v1 (obsolete) — Splinter Review
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: general → jorendorff
Status: NEW → ASSIGNED
Attachment #408651 - Flags: review?(brendan)
Blocks: 505932
No longer depends on: 505932
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 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+
http://hg.mozilla.org/tracemonkey/rev/d54c92b7f76e
Flags: blocking1.9.2?
Whiteboard: fixed-in-tracemonkey
Flags: in-testsuite+
(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.
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.
(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.
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
These patches hit m-c today, so we can see if the crashes stop tomorrow.
http://hg.mozilla.org/mozilla-central/rev/d54c92b7f76e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: blocking1.9.2? → blocking1.9.2+
blocking1.9.1: --- → ?
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: ? → ---
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.)
(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: --- → ?
(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)
(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.
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)
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: ? → -
(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.
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.
(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.
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.
(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.
(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.
Duplicate of this bug: 523998
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.
Duplicate of this bug: 534757
Summary: Shape regeneration still does not touch most empty scopes → Shape regeneration still does not touch most empty scopes [@ js_Interpret:4436]
Crash Signature: [@ js_Interpret:4436]
Group: core-security
You need to log in before you can comment on or make changes to this bug.