Last Comment Bug 366770 - Use the file's first binding if no fragment identifier is present in a binding URI
: Use the file's first binding if no fragment identifier is present in a bindin...
Status: RESOLVED FIXED
: dev-doc-complete, testcase
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9alpha3
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
: Hixie (not reading bugmail)
Mentors:
Depends on: 397596 432813 444490 507991
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-11 20:45 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2009-08-11 14:12 PDT (History)
10 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (527 bytes, text/html)
2007-01-25 11:46 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
patch (21.57 KB, patch)
2007-02-09 11:08 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch (22.67 KB, patch)
2007-02-12 11:22 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
updated to comments (23.14 KB, patch)
2007-02-23 14:45 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
test (4.77 KB, patch)
2007-02-25 10:52 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
bzbarsky: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-11 20:45:11 PST
See bug 243917 comment 6.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-01-25 11:46:46 PST
Created attachment 252801 [details]
testcase
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-02-09 11:08:02 PST
Created attachment 254551 [details] [diff] [review]
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 [:bz] 2007-02-09 13:43:13 PST
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: gavin@gavinsharp.com] 2007-02-09 13:56:22 PST
(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 [:bz] 2007-02-09 14:00:01 PST
> The "#" at the end is just treated as part of the data, right?

Ok.  Sounds good!
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-10 05:53:28 PST
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: gavin@gavinsharp.com] 2007-02-10 14:48:01 PST
(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 [:bz] 2007-02-11 13:13:48 PST
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: gavin@gavinsharp.com] 2007-02-12 11:22:56 PST
Created attachment 254843 [details] [diff] [review]
patch
Comment 10 Boris Zbarsky [:bz] 2007-02-23 12:41:06 PST
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: gavin@gavinsharp.com] 2007-02-23 14:45:48 PST
Created attachment 256220 [details] [diff] [review]
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 [:bz] 2007-02-23 21:04:48 PST
Looks good!
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-02-24 08:37:48 PST
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 [:bz] 2007-02-25 09:35:39 PST
> 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: gavin@gavinsharp.com] 2007-02-25 10:52:50 PST
Created attachment 256372 [details] [diff] [review]
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 [:bz] 2007-02-25 14:59:26 PST
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: gavin@gavinsharp.com] 2007-02-27 11:47:40 PST
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 [:mwargers] (not working for Mozilla) 2007-04-11 12:32:20 PDT
I guess it would be nice if this new feature was mentioned somewhere on mdc.
Comment 19 Jesse Ruderman 2007-09-25 15:51:25 PDT
I filed bug 397570 on this not working if the ID attribute is omitted.
Comment 20 Eric Shepherd [:sheppy] 2007-10-11 08:24:14 PDT
Can someone explain to me what this means and why it matters, so I can turn that into documentation?  :)
Comment 21 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-10-11 08:42:45 PDT
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 2007-10-11 08:46:51 PDT
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: gavin@gavinsharp.com] 2007-10-11 08:56:28 PDT
(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] 2007-10-11 09:02:16 PDT
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] 2007-10-11 17:24:30 PDT
OK, now actually setting as doc complete.
Comment 26 Jesse Ruderman 2008-04-29 18:06:50 PDT
This fix for bug 379959 made it so you can't use -moz-binding:url(data:...) from content for security reasons.

Note You need to log in before you can comment on or make changes to this bug.