Print a deprecation warning when importing XUL into content documents

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

({addon-compat})

unspecified
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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?
Flags: needinfo?(jorge)
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(bobbyholley) → needinfo?(bugs)
Oh, there's also the case of a anonymous content from an in-content XBL binding - the videocontrols binding does this, for example.
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).
Flags: needinfo?(bugs)
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.
Flags: needinfo?(jorge)
Assignee: nobody → wmccloskey
Posted patch xul-warning (obsolete) — Splinter Review
I tried the BindToTree approach smaug suggested and it seems to work.
Attachment #8457729 - Flags: review?(bugs)
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).
Attachment #8457729 - Flags: review?(bugs) → review-
This should do it.
Attachment #8457729 - Attachment is obsolete: true
Attachment #8458062 - Flags: review?(bugs)
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>
Attachment #8458062 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/be9e86108cf9
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1059707
You need to log in before you can comment on or make changes to this bug.