1291001 - Split wrap from rewrap to simplify gray unmarking semantics around the existing parameter

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: split_wrap_and_rewrap-v0.diff

This adds a bit of extra code, but I think it's a pretty dramatic simplification over what's there now. Or maybe I've been staring at it for too long. Feedback needed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeef11ad47fd

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8776685 [details] [diff] [review]
split_wrap_and_rewrap-v0.diff

Need to deal with DebugOnly related build failures. Forgot that it doesn't work smoothly with Rooted. Seems like there's not any good reason to store the global, so that should be easy to fix.

There are also debug mode mochitest failures. It looks from the assertions as if RemapWrappers is calling wrap with a null wrapper, which I did not expect. I'm going to try to get an rr trace first to verify, but it should be an easy fix as well.

Comment 2

[Boris Zbarsky [:bzbarsky]]

This seems pretty reasonable.  I'm not sure about the reifySameCompartmentWrapper naming; in a lot of cases the thing it returns is _not_ a wrapper at all, right?  Might want to think about the naming a bit.

Comment 3

[Terrence Cole [:terrence]]

Yeah, that name is a total cop-out. Reify (i.e. "make correct") is what I fall back on when I can't think of something that's otherwise a correct summary. My first name was |stripWrapperIfSameCompartment|, but that doesn't happen for window wrappers. Maybe |stripCrossCompartmentWrapperForSameCompartment|, but that's *really* long and it's still not true for the StopIteration case.

Comment 4

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64c133a32189

Comment 5

[Terrence Cole [:terrence]]

We decided on getNonWrapperObject during the GC meeting.

Comment 6

[Terrence Cole [:terrence]]

Maybe also ForCurrentCompartment.

Comment 7

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6aa47cc8f94e

Comment 8

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=449d9eca5854

Comment 9

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08729ae1e474

Comment 10

[Terrence Cole [:terrence]]

Let's see if rebasing fixes the reftest timeouts.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a547307c912

Comment 11

[Terrence Cole [:terrence]]

Yes, indeed it does!

Comment 12

[Terrence Cole [:terrence]]

Comment on attachment 8776685 [details] [diff] [review]
split_wrap_and_rewrap-v0.diff

I thought I had already gotten this reviewed and landed, but it looks like it was rebuffed by the reftests timeouts on try a month ago. Oh well, let's do it now!

Ideally Boris would be able to do this review -- we've been staring at the code side-by-side for weeks now -- but he's not accepting reviews. Instead, let's get a fresh pair of eyes on it.

Comment 13

[Terrence Cole [:terrence]]

Uh oh! Looks like this hits an assertion:

Assertion failure: IsBackgroundFinalized(a->asTenured().getAllocKind()) == IsBackgroundFinalized(b->asTenured().getAllocKind()), at /builds/slave/try-lx-d-000000000000000000000/build/src/js/src/jsobj.cpp:1547

Presumably something we did in the |existingArg| path (now rewrap) that I thought was incidental was somehow ensuring that we did not get to TradeGuts with mismatched background finalization kinds.

Comment 14

[Terrence Cole [:terrence]]

For some reason the new code stopped passing |existing| in getOrCreateWrapper (ne 3-arg wrap) into the callbacks still-3-arg wrap function. I vaguely remember having a reason for this, but it certainly looks wrong! Let's try it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b67553a1f40d

Comment 15

[Terrence Cole [:terrence]]

Attachment: missing_existing_arg-v0.diff

Success! Looks like that was indeed the bug. This is the diff, in case you are curious where the change was exactly. Otherwise the patch I'm about to post is identical to v0.

Comment 16

[Terrence Cole [:terrence]]

Attachment: split_wrap_and_rewrap-v1.diff

Comment 17

[Jon Coppeard (:jonco)]

Comment on attachment 8793939 [details] [diff] [review]
split_wrap_and_rewrap-v1.diff

Review of attachment 8793939 [details] [diff] [review]:
-----------------------------------------------------------------

This great simplification and looks good to me.  I like how this splits the case where we need to unmark grey objects off from remapping case.

::: js/src/jscompartment.cpp
@@ +330,5 @@
>      return true;
>  }
>  
>  bool
> +JSCompartment::getNonWrapperObjectForCurrentCompartment(JSContext* cx, MutableHandleObject obj)

Thanks for renaming this from the earlier patch, it's much clearer now.

@@ +336,5 @@
>      // Wrappers should really be parented to the wrapped parent of the wrapped
>      // object, but in that case a wrapped global object would have a nullptr
>      // parent without being a proper global object (JSCLASS_IS_GLOBAL). Instead,
>      // we parent all wrappers to the global object in their home compartment.
>      // This loses us some transparency, and is generally very cheesy.

I don't know what this comment means now there is no such thing as parents any more.

@@ +395,5 @@
>      MOZ_ASSERT(!IsWindow(obj));
>  
> +    return true;
> +}
> + 

nit: stray space here?

@@ +457,5 @@
> +    // not need to create or return a wrapper.
> +    if (obj->compartment() != this) {
> +        if (!getOrCreateWrapper(cx, nullptr, obj))
> +            return false;
> +    }

nit: the if condition does the opposite check to that described in the comment.  I'm fine to leave this but it is slightly confusing.

Comment 18

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #17)
> @@ +336,5 @@
> >      // Wrappers should really be parented to the wrapped parent of the wrapped
> >      // object, but in that case a wrapped global object would have a nullptr
> >      // parent without being a proper global object (JSCLASS_IS_GLOBAL). Instead,
> >      // we parent all wrappers to the global object in their home compartment.
> >      // This loses us some transparency, and is generally very cheesy.
> 
> I don't know what this comment means now there is no such thing as parents
> any more.

Wow, yeah. I don't know what sense it even makes to assert obj->global() is non-null: it even hands out a reference. I've removed that assert and changed the comment to a tepid: "Ensure that we have entered a compartment".

> @@ +395,5 @@
> >      MOZ_ASSERT(!IsWindow(obj));
> >  
> > +    return true;
> > +}
> > + 
> 
> nit: stray space here?

Thanks! I had to rebase this patch almost entirely manually because of the prior work Boris and I did to litter it with assertions and Exposures. 

> @@ +457,5 @@
> > +    // not need to create or return a wrapper.
> > +    if (obj->compartment() != this) {
> > +        if (!getOrCreateWrapper(cx, nullptr, obj))
> > +            return false;
> > +    }
> 
> nit: the if condition does the opposite check to that described in the
> comment.  I'm fine to leave this but it is slightly confusing.

Yeah, that's just awkwardly worded. I've changed it to:
    // If the reification above did not result in a same-compartment object,
    // get or create a new wrapper object in this compartment for it.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6557447e01f4
Split wrap and rewrap to simplify semantics around |existing|; r=jonco

Comment 20

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/6557447e01f4

Comment 21

[Gerry Chang [:gchang]]

Mark 51 as won't fix. If it's worth uplifting to 51, feel free to nominate it.