1027299 - Print a deprecation warning when importing XUL into content documents

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We no longer support adding XUL nodes to content documents. However, add-ons can still create XUL nodes in a chrome document and then use contentDocument.importNode to pull them into content. It's very difficult to make this sort of thing work in electrolysis.

Jorge, do you think it would be acceptable if we printed a warning that this functionality will go away in a few versions and then eventually removed it? The case I've seen is in Video Download Helper, where they create a XUL pop-up menu next to the title of videos. It seems like they could use an HTML popup just as easily. I imagine other add-ons do the same thing though. A more conservative path would be to try to use telemetry to count how often this happens (and perhaps figure out which add-ons do it).

Bobby, can you think of any other paths besides importNode where XUL nodes could end up in content documents?

Comment 1

[Bobby Holley (:bholley)]

We should do the same for adoptNode. I believe that adoptNode, importNode, createElement*, and parser-driven stuff should cover the cases here. Olli, can you confirm?

Comment 2

[Bobby Holley (:bholley)]

Oh, there's also the case of a anonymous content from an in-content XBL binding - the videocontrols binding does this, for example.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

createElement and parser driven stuff just shouldn't just work atm
http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp#297

XBL should end up using nsXBLBinding::GenerateAnonymousContent() which calls nsNodeUtils::Clone, 
so we could add some param there to suppress the warning...

Or would it be enough to add a check to XULElement::BindToTree.
If 'this' isn't in anonymous content, and OwnerDoc()->AllowXULXBL() returns false, print the warning
(asynchronously, since doing that inside BindToTree wouldn't be safe).

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

I'm in favor of the warning for a few versions and then breaking compatibility. I'm also CCing mig, since VideoDownload Helper is a fairly major add-on and I want him to have a good head start.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: xul-warning

I tried the BindToTree approach smaug suggested and it seems to work.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8457729 [details] [diff] [review]
xul-warning

I think we should do this only once per document. That limits the number of
warnings a bit.
So add the thing to nsDeprecatedOperationList and use WarnOnceAbout, and
the runnable shouldn't be posted if we have warned already so that
we don't get the performance hit so often (in case some addon is doing
something weird).

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: xul-warning v2

This should do it.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8458062 [details] [diff] [review]
xul-warning v2

>+  NS_IMETHOD Run() {
nit, { goes to its own line

>+  var browser = document.getElementById("browserelt");
>+  browser.addEventListener("load", function() {
>+    // We add some XUL to a content document. This should generate a warning.
>+    var elt = document.createElement("label");
>+    var doc = browser.contentDocument;
>+    var newElt = doc.importNode(elt, false);
>+    expectWarning(true, "appending XUL", function() {
>+      doc.documentElement.appendChild(newElt);
>+    });
>+
>+    // We add a <video> element, which contains anonymous XUL content. This should not warn.
>+    var video = doc.createElement("video")

>+    expectWarning(false, "appending video", function() {
>+      doc.documentElement.appendChild(video);
I think you should flush layout here (doc.documentElement.offsetLeft or some such) to force video
to have the UI and anonymous content


>+    });
Now that we warn only once, you should first check <video> and then <xul:label>

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be9e86108cf9

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be9e86108cf9