833412 - Remove XBL binding layering (addBinding)

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Bobby Holley (:bholley)]

Right now, privileged callers can do |document.addBinding(elem, bindingURI)| to dynamically add bindings to elements. If these elements have existing bindings, the new binding is inserted at the bottom of the prototype chain. Given how binding inheritance works, the same binding can actually end up on the prototype chain twice, with all kinds of wacky effects.

I've recently discovered a major bug in addBindings, which is that it doesn't unbind |elem| before applying the new bindings, leading to even more wacky behavior. But all this machinery appears to be unused (even on AMO mxr) except for a trivial case in nsXMLPrettyPrinter, so bz and I decided we should just remove it.

Comment 1

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

Remove XMLPrettyPrinter or make it not use AddBindings? I assume the latter.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #1)
> Remove XMLPrettyPrinter or make it not use AddBindings? I assume the latter.

You are correct.

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

Green. Uploading and flagging bz for review.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix tests that call addBinding. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make nsXMLPrettyPrinter use nsXBLService/BindingManger to load/remove bindings. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove AddBinding/RemoveBinding and remove dead code. v1

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 705236 [details] [diff] [review]
Part 1 - Fix tests that call addBinding. v1

> +++ b/content/base/test/test_bug375314.html

Can you please replace the addBinding thing by a style rule that applies a binding to the <div> and then a style flush to cause it to take effect, instead of just losing XBL test coverage?

>+++ b/content/xbl/test/test_bug398492.xul

Likewise.

r=me with those

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 705237 [details] [diff] [review]
Part 2 - Make nsXMLPrettyPrinter use nsXBLService/BindingManger to load/remove bindings. v1

OK.  So you don't need the LoadBindingDocument call because this is a chrome:// URI so will get sync-loaded no matter what, right?  Might be worth commenting that...

This is not running the constructor of the binding.  That might be ok in this case because the binding has no constructor, but please add a comment to the binding XML saying that if a constructor is added this code needs changing.

Or change this code to actually run the constructor, of course.

>+    // Apply the prettprint XBL binding.

"prettyprint"


>     nsCOMPtr<nsIDOMDocument> document = do_QueryInterface(mDocument);
>     nsCOMPtr<nsIDOMElement> rootElem;
>     document->GetDocumentElement(getter_AddRefs(rootElem));
>+    nsCOMPtr<nsIContent> rootContent = do_QueryInterface(rootElem);

How about replacing all that with:

  nsCOMPtr<Element> element = mDocument->GetDocumentElement();

?

r=me with that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 705238 [details] [diff] [review]
Part 3 - Remove AddBinding/RemoveBinding and remove dead code. v1

>+++ b/content/xbl/src/nsXBLService.cpp
>-  if (aAugmentFlag) {
...
>-  }
>   else {

That's not good.  That else is now being treated as part of the previous if.  It happens to work out, but that's an accident.

Please remove the else and curlies and outdent the body?

r=me with that

Comment 11

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/616244739680
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd652b1e555
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3098dbf4eb

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/616244739680
https://hg.mozilla.org/mozilla-central/rev/6dd652b1e555
https://hg.mozilla.org/mozilla-central/rev/fb3098dbf4eb