797304 - Reimplement MoveWrappers in terms of Orphan fixup

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Currently, when we do a document.write, we iterate over the entire old scope and move any XPCWrappedNatives (see bug 752446 comment 2 for the full dirt on why we do this). The existing implementation calls PreCreate (again) and politely asks the wrapper whether it wants to move. The edge cases here have caused us a lot of pain, so I think it might be simpler to reimplement this stuff in terms of orphan fixup (where we just trace the parent chain and follow cross-compartment wrappers).

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ba6b6f7c68cf

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Explicitly reparent the document to the new scope during document.open. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Be more aggressive when fixing up orphans. v2

To make stronger assumptions, we should dig deeper on the parent chain, and also morph any slim wrappers. This is slightly slower, but not much, since this stuff only gets called for HTML documents, and the parent chains there tend to be short. Moreover, this only gets called during document.open(), where performance doesn't matter so much.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Add special handling for nuked XUL elements in orphan fixup. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Replace MoveWrapper nonsense with orphan fixup. v1

Comment 6

[Bobby Holley (:bholley)]

Looks green! Uploaded patches and flagged for review.

Comment 7

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Comment on attachment 667459 [details] [diff] [review]
Part 3 - Add special handling for nuked XUL elements in orphan fixup. v1

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

Sounds like a clever workaround!

Comment 8

[Bobby Holley (:bholley)]

Peter, is this something you have time for in the near future? I think this is a good time in the cycle to land this. If you don't have time, I'll try to find another reviewer.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 667457 [details] [diff] [review]
Part 1 - Explicitly reparent the document to the new scope during document.open. v1

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

::: content/html/document/src/nsHTMLDocument.cpp
@@ +1515,5 @@
>      SetIsInitialDocument(false);
>  
>      nsCOMPtr<nsIScriptGlobalObject> newScope(do_QueryReferent(mScopeObject));
>      if (oldScope && newScope != oldScope) {
> +      nsCOMPtr<nsISupports> doc = do_QueryObject(this);

static cast to nsINode* should work. We make that assumption all over content/.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 667458 [details] [diff] [review]
Part 2 - Be more aggressive when fixing up orphans. v2

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

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ -1759,2 @@
>      nsresult rv;
> -    XPCWrappedNative *parentWrapper = GetParentWrapper();

Seems like this is the only caller for GetParentWrapper, remove it?

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 667459 [details] [diff] [review]
Part 3 - Add special handling for nuked XUL elements in orphan fixup. v1

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

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1774,5 @@
>  
> +    // There's one little nasty twist here. For reasons described in bug 752764,
> +    // we nuke SOW-ed objects after transplanting them. This means that XUL
> +    // elements, which are parented directly to their parent element, can end up
> +    // with a nuked proxy n the parent chain, depending on the order of fixup.

*o*n/*i*n the parent chain?

@@ +1779,5 @@
> +    // Because the proxy is nuked, we can't follow it anywhere. But we _can_ find
> +    // the new wrapper for the underlying XUL parent, which is exactly what
> +    // PreCreate does for XUL elements. So do that here.
> +    if (MOZ_UNLIKELY(JS_IsDeadWrapper(parentObj))) {
> +        MOZ_ASSERT(!strcmp(js::GetObjectClass(mFlatJSObject)->name, "XULElement"));

Nuke the assert and change the comments to talk about |nodes parented to an element|.

Comment 12

[Bobby Holley (:bholley)]

thanks peter!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b57ffb225317
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9b7d6c1396
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a812a1ef0bfc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c37ac98907d1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b57ffb225317
https://hg.mozilla.org/mozilla-central/rev/ce9b7d6c1396
https://hg.mozilla.org/mozilla-central/rev/a812a1ef0bfc
https://hg.mozilla.org/mozilla-central/rev/c37ac98907d1