591198 - With XBL disabled, -moz-binding should not make element disappear

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Scoobidiver (away)]

Attachment: Driver list screenshot with no html link

Build : Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100826 Minefield/4.0b5pre

Steps to reproduce :
1. Go to the ref URL
2. Unable to select a driver because the onclick content is not displayed

This issue is recent as it works fine in FF 4.0 b4

Comment 1

[Scoobidiver (away)]

Attachment: Driver list screenshot (FF 4.0 b4)

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

Did a quick Check to verify that this isn't an User Agent Issue.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9a623cc1c5e7&tochange=4a6ee9e82945

The site has a binding attached to the <h3> around that clickable text:

  -moz-binding: url("moz-wordwrap.xml#wordwrap");

and with the changes in bug 546857 we don't create a frame for this stuff anymore... I wonder whether we should just ignore the existence of the binding instead or something?  But really, this site should just stop using nonstandard stuff like that.

Comment 4

[Martijn Wargers (dead)]

(In reply to comment #3)
> anymore... I wonder whether we should just ignore the existence of the binding
> instead or something?  But really, this site should just stop using nonstandard
> stuff like that.

Yes, you should ignore the existance of the binding. And I would even argue that you should get a css parsing error for the -moz-binding rule, nowadays.
I'm using something similar on http://www.kantjils.nl/ for the links on the left column. In current trunk builds, those links are not showing up anymore.
And no, I don't intend on stopping using non-standard stuff like that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Just not parsing moz-binding from untrusted origins might be a good way to go, yes.  Jonas, David, thoughts?

Comment 6

[Scoobidiver (away)]

What about the absence of version in version column in the ref URL ?
Is it a consequence of binding ignoring ?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #5)
> Just not parsing moz-binding from untrusted origins might be a good way to go,
> yes.  Jonas, David, thoughts?

sounds fine to me

Comment 9

[Boris Zbarsky [:bzbarsky]]

I think this needs to block beta6.

Comment 10

[Mike Shaver (:shaver emeritus)]

Gotta get this in b6, to see what else it breaks.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This was a pretty trivial fix. The old code assumed that if a LoadBindings call failed, that it failed because we were loading the binding document asynchronously. So we simply waited with rendering the element at all and assumed that the "xbl load is done" code would render things for us.

This patch makes us return a special error code when XBL is blocked for security reasons. When this happens we go ahead and render the element as normal.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

qref'ed the wrong way. This includes added files.

Comment 14

[Martijn Wargers (dead)]

So I guess it's ok that -moz-binding is accepted in css, but that it doesn't do anything?
Should dom methods like getAnonymousNodes, getAnonymousElementByAttribute, loadBindingDocument still work when xbl is disabled?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #14)
> So I guess it's ok that -moz-binding is accepted in css, but that it doesn't do
> anything?

It's not ideal, but I also don't think it's a big deal.

I guess it might be a good idea to warn in the cases when we're blocking XUL and/or XBL though. I'll see if I can come up with a patch to do that before release.

> Should dom methods like getAnonymousNodes, getAnonymousElementByAttribute,
> loadBindingDocument still work when xbl is disabled?

For now they have to (possibly except for loadBindingDocument). We use these functions in things like video controls and maybe marquee. I think.

But when webpage code access them, you'll generally never get meaningful results, so there shouldn't be any security concerns.

Comment 17

[TimoT]

I think this broke new tab and tab view buttons. I can do both via kb shortcuts but actually clicking the buttons doesn't do anything.

Comment 18

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

This landed:
http://hg.mozilla.org/mozilla-central/rev/2e55203d7b80
and then the added files were landed:
http://hg.mozilla.org/mozilla-central/rev/a6ee83aa638e
and then I backed the whole thing out on jst's instructions (because his tree was messed up):
http://hg.mozilla.org/mozilla-central/rev/59244235c85b
http://hg.mozilla.org/mozilla-central/rev/811c34d504ed
http://hg.mozilla.org/mozilla-central/rev/5ef1dd2885fb
http://hg.mozilla.org/mozilla-central/rev/4405a670d699

Comment 19

[Chris Coulson]

I confirm what Timo says too, the buttons in the tabbar don't respond to mouse clicks in the current Ubuntu daily builds. I did a hg bisect which narrowed down the bad revision to either http://hg.mozilla.org/mozilla-central/rev/2e55203d7b80 or http://hg.mozilla.org/mozilla-central/rev/a6ee83aa638e, but I see both of these have been backed out already anyway

Due to skipped revisions, the first bad revision could be any of:
changeset:   53750:2e55203d7b80
user:        Jonas Sicking <[e-mail]>
date:        Tue Sep 14 02:22:06 2010 +0200
summary:     Bug 591198 - Don't make elements disappear if XBL bindings fail due to security restrictions. r=jst a2.0=blocking

changeset:   53751:a6ee83aa638e
user:        Mounir Lamouri <[e-mail]>
date:        Tue Sep 14 02:43:39 2010 +0200
summary:     Bustage: pushing missing files. r+a=bustage

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

This is a more conservative fix. The old patch moved some code into the |if (we have a new binding)| test, which looked safe, but knowing the state of our XBL code, I should have known that it wasn't :(

Comment 22

[Boris Zbarsky [:bzbarsky]]

Hmm.  That looked safe to me too.  When is it not?

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't know. It appears it is moving this line to inside the |if| that breaks things:

http://hg.mozilla.org/mozilla-central/annotate/11e4faad5545/layout/base/nsCSSFrameConstructor.cpp#l5113

Comment 24

[Boris Zbarsky [:bzbarsky]]

Oh, right.  If the binding is already applied to the node, LoadBindings won't return the binding (since it doesn't need its ctor to run), but it will still affect the tag/namespace...

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Stuck this time
http://hg.mozilla.org/mozilla-central/rev/5ba6fb8ab3ab

Comment 26

[Mardeg]

I take it this is what broke the "XBL and ZEN garden" demo here?:
http://weblogs.mozillazine.org/hyatt/demo/tranquille.html
The info on that page may need updating then.

Comment 27

[Martijn Wargers (dead)]

-moz-binding: none still works from content, fwiw. And I assume that's also the case when pointing to certain chrome binding urls.