Jan de Mooij [:jandem] Reporter
	Description • 6 years ago

Bug 1514210 is failing this assert on try:

  JS::Compartment* oldCompartment = js::GetObjectCompartment(oldParent);
  JS::Compartment* newCompartment = js::GetObjectCompartment(newParent);
  if (oldCompartment == newCompartment) {
    MOZ_ASSERT(oldParent == newParent);
    return;
  }

https://searchfox.org/mozilla-central/rev/fd32b3a6fa3eff1468311f6fcf32b45c117136df/dom/bindings/BindingUtils.cpp#2157

This code relies on CPG and needs to be fixed somehow.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 6 years ago

This code dates back to when we had a concept of parent as distinct from the
concept of global.  It was comparing compartments back then because in the
same-compartment case it would just JS_SetParent and return.  When we got rid of
the concept of parents, the code was left as-is, even though at that point we
could just as easily compare the two globals.

I believe that in the same-compartment-different-globals case this is safe,
because in that case JS_TransplantObject will just keep using the original
object allocation but JSObject::swap it with the new object, so that it will
pick up the new global.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 6 years ago

We're not changing parents; we're changing globals.  Let's be clear about that.

	Jan de Mooij [:jandem] Reporter
	Comment 3 • 6 years ago

Comment on attachment 9031512 [details] [diff] [review]
part 1.  Stop relying on compartment-per-global in ReparentWrapper

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

Makes sense. Thank you for fixing this and the other bug!

	Peter Van der Beken [:peterv]
	Comment 4 • 6 years ago

Comment on attachment 9031513 [details] [diff] [review]
part 2.  Update naming in ReparentWrapper to reflect reality

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

::: dom/bindings/BindingUtils.h
@@ +2329,5 @@
> +// Given a DOM reflector aObj, give its underlying DOM object a reflector in
> +// whatever global that underlying DOM object now thinks it should be in.  If
> +// this is in a different compartment from aObj, aObj will become a
> +// cross-compatment wrapper for the new object.  Otherwise, aObj will become the
> +// new object.

New object could be the same object, right? Not sure if we should specify that.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 6 years ago

> New object could be the same object, right?

Yes.  Added:

  If the new global is the same as the old global, we just keep using the same object.

	Pulsebot
	Comment 6 • 6 years ago

Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e52c32b7874
part 1.  Stop relying on compartment-per-global in ReparentWrapper. r=peterv,jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8f0ee812c88
part 2.  Update naming in ReparentWrapper to reflect reality.  r=peterv

	Razvan Maries
	Comment 7 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8e52c32b7874
https://hg.mozilla.org/mozilla-central/rev/c8f0ee812c88