Jason Orendorff [:jorendorff] Assignee
	Description • 15 years ago

+++ 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 1 • 15 years ago

BTW, this might very well be the cause of topcrash bug 519363.

	Jason Orendorff [:jorendorff] 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

	Jason Orendorff [:jorendorff] 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.

	Brendan Eich [: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

	Jason Orendorff [:jorendorff] Assignee
	Comment 5 • 15 years ago

http://hg.mozilla.org/tracemonkey/rev/d54c92b7f76e

	Bob Clary [:bc] (inactive)
	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.

	Bob Clary [:bc] (inactive)
	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.

	Bob Clary [:bc] (inactive)
	Comment 8 • 15 years ago

http://hg.mozilla.org/tracemonkey/rev/5298ad5cc9aa

	Jason Orendorff [:jorendorff] 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.

	Bob Clary [:bc] (inactive)
	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

	David Mandelin [:dmandelin]
	Comment 11 • 15 years ago

These patches hit m-c today, so we can see if the crashes stop tomorrow.

	Robert Sayre
	Comment 12 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/d54c92b7f76e

	Daniel Veditz [:dveditz]
	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

	David Mandelin [:dmandelin]
	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.)

	Samuel Sidler (old account; do not CC)
	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?

	David Mandelin [:dmandelin]
	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)

	Jason Orendorff [:jorendorff] 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.

	Daniel Veditz [:dveditz]
	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)

	Samuel Sidler (old account; do not CC)
	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.

	Robert Sayre
	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.

	Samuel Sidler (old account; do not CC)
	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.

	Robert Sayre
	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.

	Samuel Sidler (old account; do not CC)
	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.

	Robert Sayre
	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.

	Jason Orendorff [:jorendorff] 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.

	David Mandelin [:dmandelin]
	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.