1590526 - Temporarily allow node adoption across different docGroups for the content/content case of WebExtensions

Status: VERIFIED FIXED

(Core :: XPConnect, enhancement, P1)

Description

[Tomislav Jovanovic :zombie]

+++ This bug was initially created as a clone of Bug #1467970 +++

Basically, reverse the effects of patch in bug 1467970 across docGroups which are both content. I believe this is currently only possible in web extensions, and is already breaking them on Beta 71.

Comment 1

[Tomislav Jovanovic :zombie]

If this was less time sensitive, I would have tried to write the patch myself, but since it would be great to get this uplifted to 71 soon, Sean, could you please take this, or suggest someone else who could?

Comment 2

[Sean Feng [:sefeng]]

Yeah, I'll do it, thanks for the bug!

Comment 3

[Tomislav Jovanovic :zombie]

Err, removed dependency by accident, not sure if depends on bug 1467970 is right, but feel free add it back.

(In reply to Sean Feng [:sefeng] from comment #2)

Yeah, I'll do it, thanks for the bug!

Thank you, let me know if you need any help with a test case or anything else.

Comment 4

[Tomislav Jovanovic :zombie]

Finally managed to reproduce and reduce the test case from bug 1576508 comment 25:

let xhr = new XMLHttpRequest();
xhr.responseType = 'document';
xhr.open('GET', chrome.extension.getURL('/blank.html'));

xhr.onload = function() {
    let doc = xhr.response;
    let node = doc.body.cloneNode(true); // original
    // let node = document.adoptNode(doc.body); // this works

    document.body.appendChild(node); // throws with original
    console.log("done");
}
xhr.send();
console.log("sent");

This looks much more innocuous, and probably something that we will still want to keep working. I understand the proper way to do it would be to use document.adoptNode() or .importNode(), but we never enforced that for the web at large, and would also be a surprising exception for web extensions.

I feel this should continue to work similarly to a standard web page XHR with proper CORS permissions.

Comment 5

[Boris Zbarsky [:bzbarsky]]

// let node = document.adoptNode(doc.body); // this works

That is ... really surprising. I would have expected the changes in bug 1467970 would make that explicit adoptNode call throw!

Comment 6

[Sean Feng [:sefeng]]

Attachment: Bug 1590526 - Temporarily allow node adoption across different docGroups for the content/content case

As web extensions rely on this node adoption between content to content
documents, we want to continue allowing this capability to work for now.

Comment 7

[Tomislav Jovanovic :zombie]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5)

// let node = document.adoptNode(doc.body); // this works

That is ... really surprising. I would have expected the changes in bug 1467970 would make that explicit adoptNode call throw!

I'm sorry, I was testing several different configurations across several versions (and fighting cached scripts), so I messed up. The importNode version works in current nightly:

// let node = document.importNode(doc.body, true); // this works

adoptNode throwing would be expected according to changes from bug 1467970, but seems like something that should definitely continue to work for the XHR test case.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yes, I agree that it should keep working for the XHR testcase, and similar for use of DOMParser.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78c33df33145
Temporarily allow node adoption across different docGroups for the content/content case r=smaug,zombie

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/78c33df33145

Comment 11

[Sean Feng [:sefeng]]

Comment on attachment 9103725 [details]
Bug 1590526 - Temporarily allow node adoption across different docGroups for the content/content case

Beta/Release Uplift Approval Request

• User impact if declined: End users may experience some extensions stop to work
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes. Unzip the adoptNodeXdocGroup.zip attachment and load it as a temporary addon from about:debugging. Go to example.com, the content of the injected iframe should be bold.
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Medium, the code itself isn't complex, but it impacts an important feature as extensions.
• String changes made/needed: None

Comment 12

[Sean Feng [:sefeng]]

Attachment: adoptNodeXdocGroup.zip

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9103725 [details]
Bug 1590526 - Temporarily allow node adoption across different docGroups for the content/content case

Fix for a P1, with tests, uplift approved for 71 beta 5. let's have QA verify the fix on beta.

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1542e80327c2

Comment 15

[Peter Magyari (Desktop QA)]

Verified fixed using Firefox Nighly 72.0a1 (20191027212548) on Window 10, MacOS 10.15 and Ubuntu 18.04.
I'll be back to verify it on 71.0b5 as well.

Comment 16

[Tomislav Jovanovic :zombie]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

Yes, I agree that it should keep working for the XHR testcase, and similar for use of DOMParser.

Right, though I meant the XHR use case should be something we support going forward, even after Fission.

Comment 17

[Peter Magyari (Desktop QA)]

Verified fixed on Firefox Beta 71.0b5 (20191028110005) on Windows 10, MacOS 10.15 and Ubuntu 18.04