366770 - Use the file's first binding if no fragment identifier is present in a binding URI

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

See bug 243917 comment 6.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: testcase

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

I added a new method on nsIXBLDocumentInfo called SetFirstPrototypeBinding to let it keep track of the document's first binding. To support data: URIs as binding references, I needed to have nsXBLPrototypeBinding keep it's ID separately from the URI (since data: URIs are not nsIURLs, so we can't use the ref).

I know that both of your review queues are pretty full already, but I don't know of anyone else who can review this. Suggestions welcome :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

You could try bryner or hyatt, but I dunno how far that would get you... ;)

How urgent is this?  I'd really like to do enn's thing ASAP, but if you need this very soon I can make time for it.  If not, I might be a week or two, at worst.

Out of curiosity, what's the plan for data: URIs which have a '#' on the end, with this patch?

Also, you needed to save the ref for GetID()? That seems to be used exactly once, in a debug printf.  I say we just nix it in favor of the URI in that printf.  ;)

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)
> How urgent is this?  I'd really like to do enn's thing ASAP, but if you need
> this very soon I can make time for it.  If not, I might be a week or two, at
> worst.

It's not urgent at all. Two weeks wait is fine.

> Out of curiosity, what's the plan for data: URIs which have a '#' on the end,
> with this patch?

The "#" at the end is just treated as part of the data, right? nsSimpleURIs aren't nsIURLs, and that's what XBL uses to get the fragment.

> Also, you needed to save the ref for GetID()? That seems to be used exactly
> once, in a debug printf.  I say we just nix it in favor of the URI in that
> printf.  ;)

Ah, good point. I'll do that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> The "#" at the end is just treated as part of the data, right?

Ok.  Sounds good!

Comment 6

[Martijn Wargers (dead)]

Are there security issues that need to be considered?
I mean, this would allow javascript to be used inside css, javascript which is able to reach the whole document.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> Are there security issues that need to be considered?
> I mean, this would allow javascript to be used inside css, javascript which is
> able to reach the whole document.

I don't think there are, though I'm not exactly 100% sure. nsXBLService::LoadBindings does security checks before ever calling nsXBLService::GetBinding, so the code I'm changing runs after all of that's been done. In this case I don't think the javascript: URIs are able to reach the document being loaded, in case you're worried of document mutations while loading bindings or something like that. I'm not exactly sure how javascript: URIs are handled in this case, though, so maybe I'm wrong. I suspect CSS url(javascript:...) security is handled at a different level than the code I'm changing (I certainly hope so, anyways).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Martijn, this wouldn't allow anything that's not already allowed -- you can already link to an http:// moz-binding, and you can already have javascript: URIs in CSS (e.g. background-image).

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 254843 [details] [diff] [review]
patch

>Index: content/xbl/src/nsXBLBinding.cpp
>-    nsCAutoString id;
>-    mPrototypeBinding->GetID(id);
>-    nsCAutoString message("An XBL Binding with an id of ");
>-    message += id;
>-    message += " and found in the file ";
>+    nsCAutoString message("An XBL Binding with URI ");
>     nsCAutoString uri;
>     mPrototypeBinding->DocURI()->GetSpec(uri);

How about using mPrototypeBinding->BindingURI() instead, so the id isn't lost if there is one?

>Index: content/xbl/src/nsXBLDocumentInfo.h
>+  nsXBLPrototypeBinding* mFirstBinding;

Worth documenting that this is a non-owning pointer and that the mFirstBinding is just owned by the table like all the other bindings.

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp
>-  nsresult rv = NS_NewURI(getter_AddRefs(uri),

This could probably use a comment explaining why it's done this way.

And why not just Clone() directly into mBindingURI?

With those nits fixed, r+sr=bzbarsky

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated to comments

(In reply to comment #10)
> (From update of attachment 254843 [details] [diff] [review])
> >Index: content/xbl/src/nsXBLBinding.cpp
> >-    nsCAutoString id;
> >-    mPrototypeBinding->GetID(id);
> >-    nsCAutoString message("An XBL Binding with an id of ");
> >-    message += id;
> >-    message += " and found in the file ";
> >+    nsCAutoString message("An XBL Binding with URI ");
> >     nsCAutoString uri;
> >     mPrototypeBinding->DocURI()->GetSpec(uri);
> 
> How about using mPrototypeBinding->BindingURI() instead, so the id isn't lost
> if there is one?

Good idea, made that change.

> >Index: content/xbl/src/nsXBLDocumentInfo.h
> >+  nsXBLPrototypeBinding* mFirstBinding;
> 
> Worth documenting that this is a non-owning pointer and that the mFirstBinding
> is just owned by the table like all the other bindings.

Done

> >Index: content/xbl/src/nsXBLPrototypeBinding.cpp
> >-  nsresult rv = NS_NewURI(getter_AddRefs(uri),
> 
> This could probably use a comment explaining why it's done this way.
> 
> And why not just Clone() directly into mBindingURI?

Made both changes, let me know if the comment is OK?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Looks good!

Comment 13

[:Gavin Sharp [email: [e-mail]]]

mozilla/content/xbl/Makefile.in 	1.5
mozilla/content/xbl/public/nsIXBLDocumentInfo.h 	1.18
mozilla/content/xbl/src/nsXBLBinding.cpp 	1.226
mozilla/content/xbl/src/nsXBLContentSink.cpp 	1.77
mozilla/content/xbl/src/nsXBLContentSink.h 	1.28
mozilla/content/xbl/src/nsXBLDocumentInfo.cpp 	1.52
mozilla/content/xbl/src/nsXBLDocumentInfo.h 	1.15
mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp 	1.125
mozilla/content/xbl/src/nsXBLPrototypeBinding.h 	1.46
mozilla/content/xbl/src/nsXBLService.cpp 	1.228
mozilla/content/xbl/src/nsXBLService.h 	1.46

I'd like to add a test for this, but I'm not sure how. I can't think of a way to test this with mochitest. A reftest with the testcase in the bug would probably be easiest, but since there are no reftests outside of layout I'm not sure how to proceed. How do the test boxes find the reftests? Is it just hardcoded to look at layout/reftests/reftest.list? Should I just add the test somewhere under layout/reftests, even though it doesn't have anything to do with layout?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I can't think of a way to test this with mochitest.

Attach a binding via a data: URI.  Have the binding constructor touch some variable in the document scope?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Attachment: test

(In reply to comment #14)
> > I can't think of a way to test this with mochitest.
> 
> Attach a binding via a data: URI.  Have the binding constructor touch some
> variable in the document scope?

Ah, why didn't I think of that? :)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 256372 [details] [diff] [review]
test

Maybe toss the contents of that data: URI into a comment somewhere so it's easy to tell what it's doing?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Landed the test with the data: URI in a comment.

mozilla/content/xbl/Makefile.in 	1.7
mozilla/content/xbl/test/Makefile.in 	1.1
mozilla/content/xbl/test/test_bug366770.html 	1.1

Comment 18

[Martijn Wargers (dead)]

I guess it would be nice if this new feature was mentioned somewhere on mdc.

Comment 19

[Jesse Ruderman]

I filed bug 397570 on this not working if the ID attribute is omitted.

Comment 20

[Eric Shepherd [:sheppy]]

Can someone explain to me what this means and why it matters, so I can turn that into documentation?  :)

Comment 21

[Martijn Wargers (dead)]

You don't need to add a fragment identifier in the url referencing the binding, in that case the first binding in the list with bindings in the binding file is used.
It also makes it possible to use data urls for bindings, which is very convenient for making testcases.

Comment 22

[Wladimir Palant]

I will try. Gavin uses the following XBL binding in his testcase:

<bindings id="xbltestBindings" xmlns="http://www.mozilla.org/xbl">
  <binding id="xbltest"><content>PASS</content></binding>
</bindings>

Usually you would attach it to an element with a CSS rule like this one:

#element {
  -moz-binding: url(http://foo.com/binding.xml#xbltest);
}

But the problem here is that we don't want to have the binding as a separate file - data: URLs are good for specifying content inline. So we want to write:

#element {
  -moz-binding: url(data:text/xml,<binding%20id=%22xbltestBindings%22...);
}

Only that this way the "#xbltest" part will be missing (data: URLs cannot have fragment identifiers) and Gecko will not know which binding to use (more than one can be specified in one file). Until now Gecko would simply ignore such an URL as "invalid". Now it will take the first binding available instead. Note that the binding still needs an id attribute even though it isn't addressed by this ID, this identifier is required internally.

Pretty much every XBL testcase uses this feature already, it makes creating an additional XML file for each testcase unnecessary.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(Martijn and Wladimir mid-aired me and provided their own clear explanations, but I spent time typing this comment up so I figured I'd post it anyways :)

(In reply to comment #20)
> Can someone explain to me what this means and why it matters, so I can turn
> that into documentation?  :)

XBL used to require a fragment identifier to point to a specific binding ID in the XBL document referred to in -moz-binding rules (so "-moz-binding: url(http://foo.com/binding.xml#mybinding)" would point to the binding with ID "mybinding" in binding.xml). We've removed that requirement by having the code use the first binding defined in the XBL document if no fragment identifier is present. This makes it possible to use data: URIs in -moz-binding rules, which was previously impossible because data: URIs don't support fragment identifiers.

The test (attachment 256372 [details] [diff] [review]) shows an example of how this functionality can be used (notice the -moz-binding rule that uses a data: URI). It's mostly useful for people who are creating standalone testcases and want to avoid having to refer to an external file, or having their document include the XBL and self-reference. Bug 243917, and bug 243917 comment 6 in particular, give a bit more context.

Comment 24

[Eric Shepherd [:sheppy]]

Thanks for the awesome explanations!  I've added some information into this section of the XBL reference.  Let me know if there's anything sucky about it:

http://developer.mozilla.org/en/docs/XBL:XBL_1.0_Reference:Elements#binding

Marking this as doc-complete.

Comment 25

[Eric Shepherd [:sheppy]]

OK, now actually setting as doc complete.

Comment 26

[Jesse Ruderman]

This fix for bug 379959 made it so you can't use -moz-binding:url(data:...) from content for security reasons.