378518 - Remove support for tag names in XBL extends attribute

Status: RESOLVED FIXED

(Core :: XBL, defect, P2)

Description

[Jesse Ruderman]

XBL lets you do crazy stuff like extends="html:input".  This usually causes Firefox to crash.  Jonas Sicking says he and bz think we should remove support for that.  (We'd continue to allow the "extends" attribute to refer to other XBL bindings, just not to tag names.)

Comment 1

[Jesse Ruderman]

If this is *not* going to be fixed for Gecko 1.9, I'd like to know so I can start Testing It Hard.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So we have, at the moment, 57 consumers of this functionality in the tree:

/toolkit/content/widgets/splitter.xml, line 7 -- <binding id="splitter" extends="xul:splitter">
/toolkit/content/widgets/splitter.xml, line 13 -- <binding id="grippy" extends="xul:button">
/toolkit/content/widgets/textbox.xml, line 14 -- <binding id="textbox" extends="xul:box">
/toolkit/content/widgets/popup.xml, line 236 -- <binding id="resizerbase" extends="xul:box">
/toolkit/content/widgets/stringbundle.xml, line 7 -- <binding id="stringbundleset" extends="xul:box"/>
/toolkit/content/widgets/stringbundle.xml, line 9 -- <binding id="stringbundle" extends="xul:spacer">
/toolkit/content/widgets/general.xml, line 352 -- <binding id="dropmarker" extends="xul:button">
/toolkit/content/widgets/scale.xml, line 8 -- <binding id="scalethumb" extends="xul:button">
/toolkit/content/widgets/scrollbar.xml, line 8 -- <binding id="thumb" extends="xul:button">
/toolkit/content/widgets/browser.xml, line 50 -- <binding id="browser" extends="xul:browser">
/toolkit/components/console/content/consoleBindings.xml, line 10 -- <binding id="console-box" extends="xul:box">
/toolkit/components/console/content/consoleBindings.xml, line 329 -- <binding id="error" extends="xul:box">
/toolkit/components/console/content/consoleBindings.xml, line 389 -- <binding id="message" extends="xul:box">
/toolkit/components/console/content/consoleBindings.xml, line 412 -- <binding id="console-error-source" extends="xul:box">
/suite/common/sidebar/sidebarBindings.xml, line 21 -- <binding id="sidebar-header-box" extends="xul:box">
/themes/modern/global/globalBindings.xml, line 44 -- <binding id="row-iconic" extends="xul:row">
/themes/modern/communicator/sidebar/sidebarBindings.xml, line 8 -- <binding id="sidebar-tab" extends="xul:button">
/themes/modern/communicator/sidebar/sidebarBindings.xml, line 24 -- <binding id="DEAD" extends="xul:button">
/themes/classic/communicator/communicatorBindings.xml, line 9 -- <binding id="row-iconic" extends="xul:row">
/themes/classic/communicator/sidebar/sidebarBindings.xml, line 8 -- <binding id="sbtab" extends="xul:button">
/browser/themes/winstripe/browser/browser.xml, line 11 -- <binding id="autocomplete-security-wrapper" extends="xul:hbox">
/layout/forms/resources/content/radio.xml, line 43 -- <binding id="radio" extends="xul:button">
/layout/forms/resources/content/select.xml, line 145 -- <binding id="select-size" extends="xul:tree">
/layout/forms/resources/content/select.xml, line 750 -- <binding id="select" extends="xul:menu">
/layout/forms/resources/content/select.xml, line 805 -- <binding id="select-option" extends="xul:menuitem">
/layout/forms/resources/content/checkbox.xml, line 43 -- <binding id="checkbox" extends="xul:image"/>
/extensions/xforms/resources/content/widgets-xul.xml, line 314 -- <binding id="dropmarker" extends="xul:button">
/xpfe/global/resources/content/bindings/splitter.xml, line 7 -- <binding id="splitter" extends="xul:splitter">
/xpfe/global/resources/content/bindings/splitter.xml, line 13 -- <binding id="grippy" extends="xul:button">
/xpfe/global/resources/content/bindings/textbox.xml, line 14 -- <binding id="textbox" extends="xul:box">
/xpfe/global/resources/content/bindings/popup.xml, line 236 -- <binding id="resizerbase" extends="xul:box">
/xpfe/global/resources/content/bindings/stringbundle.xml, line 7 -- <binding id="stringbundleset" extends="xul:box"/>
/xpfe/global/resources/content/bindings/stringbundle.xml, line 9 -- <binding id="stringbundle" extends="xul:spacer">
/xpfe/global/resources/content/bindings/general.xml, line 352 -- <binding id="dropmarker" extends="xul:button">
/xpfe/global/resources/content/bindings/scale.xml, line 8 -- <binding id="scalethumb" extends="xul:button">
/xpfe/global/resources/content/bindings/scrollbar.xml, line 8 -- <binding id="thumb" extends="xul:button">
/xpfe/global/resources/content/bindings/browser.xml, line 44 -- <binding id="browser" extends="xul:browser">
/xpfe/components/console/resources/content/consoleBindings.xml, line 10 -- <binding id="console-box" extends="xul:box">
/xpfe/components/console/resources/content/consoleBindings.xml, line 293 -- <binding id="error" extends="xul:box">
/xpfe/components/console/resources/content/consoleBindings.xml, line 354 -- <binding id="message" extends="xul:box">
/xpfe/components/console/resources/content/consoleBindings.xml, line 377 -- <binding id="console-error-source" extends="xul:box">
/calendar/prototypes/wcap/sun-calendar-event-dialog-recurrence-preview.xml, line 44 -- <binding id="recurrence-calendar" extends="xul:box">
/calendar/prototypes/wcap/sun-calendar-event-dialog-recurrence-preview.xml, line 293 -- <binding id="recurrence-preview" extends="xul:box">
/calendar/prototypes/wcap/sun-calendar-event-dialog-recurrence-datepicker.xml, line 48 -- <binding id="datepicker-box" extends="xul:text">
/calendar/prototypes/wcap/sun-calendar-event-dialog-recurrence-datepicker.xml, line 60 -- <binding id="datepicker-weekday" extends="xul:box">
/calendar/prototypes/wcap/sun-calendar-event-dialog-recurrence-datepicker.xml, line 169 -- <binding id="datepicker-monthday" extends="xul:box">
/calendar/prototypes/wcap/sun-calendar-event-dialog-freebusy.xml, line 51 -- <binding id="freebusy-box" extends="xul:text">
/calendar/prototypes/wcap/sun-calendar-event-dialog-freebusy.xml, line 62 -- <binding id="scroll-container" extends="xul:box">
/calendar/prototypes/wcap/sun-calendar-event-dialog-freebusy.xml, line 102 -- <binding id="freebusy-day" extends="xul:box">
/calendar/prototypes/wcap/sun-calendar-event-dialog-freebusy.xml, line 280 -- <binding id="freebusy-timebar" extends="xul:box">
/calendar/prototypes/wcap/sun-calendar-event-dialog-freebusy.xml, line 629 -- <binding id="freebusy-row" extends="xul:box">
/calendar/resources/content/datetimepickers/minimonth.xml, line 34 -- <binding id="minimonth" extends="xul:box" xbl:inherits="onchange,onmonthchange,onpopuplisthidden">
/calendar/resources/content/datetimepickers/minimonth.xml, line 679 -- <binding id="minimonth-day" extends="xul:text">
/calendar/resources/content/datetimepickers/datetimepickers.xml, line 629 -- <binding id="timepicker-grids" extends="xul:box">
/mailnews/base/resources/content/mailWidgets.xml, line 10 -- <binding id="dummy" extends="xul:box"/>
/mailnews/base/resources/content/mailWidgets.xml, line 947 -- <binding id="search-menulist-abstract" name="searchMenulistAbstract" extends="xul:box">
/mailnews/base/resources/content/mailWidgets.xml, line 1586 -- <binding id="searchterm" name="searchTerm" extends="xul:box">

I suppose we could white-list a small list of namespace/tag combinations to be used in extends="" if the corresponding frames don't make any assumptions about their mContent... (like only the XUL namespace and very few tag names).  We could also require that this is only done with bindings attached to XUL elements.

I think that combination could be safe, and easier to do than trying to fix all these consumers in some other way.  :(

That said, if we do have a clean way to eliminate this mess completely, that would rock.

Comment 3

[Neil Deakin]

It should perhaps only list the 20 or so XUL tags used in ConstructXULFrame. Even then, I think only 'button', 'menu', 'menupopup' and 'spacer' are actually needed. (although the xbl form controls code uses some others). But that doesn't mean they aren't used outside of Mozilla code. All of the extends='xul:box' references in the code lines above should just be removed, as well as a few others. 

Comment 4

[georgi - hopefully not receiving bugspam]

what about an option |disallow xbl| from userland?

probably the web will be usable and chrome will be unaffected.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's an interesting question.  Until recently, this hasn't so much been an option.  At this point we _could_ do it.  The only question is how many apps it would break.

Comment 6

[georgi - hopefully not receiving bugspam]

(In reply to comment #5)
> The only question is how many apps it would break.
> 

probably not exactly "apps" but "unprivileged web stuff".

not many people cry that "extends" in xbl causes SEGV.

Comment 7

[georgi - hopefully not receiving bugspam]

(In reply to comment #6)
> (In reply to comment #5)
> > The only question is how many apps it would break.
> > 
> 
> "unprivileged web stuff".
> 

"disable xbl option" is less than clever.

to put it this way:

for standard compliance reasons and the unlimited power of XBL usage of XBL from luserland requires universalbrowserwrite privilege.

 

Comment 8

[Boris Zbarsky [:bzbarsky]]

Unfortunately, there's no way to request that privilege in declarative content (e.g. CSS).

And I do mean "apps" as in "intranet apps"...

Yes, I would not have made XBL available to untrusted content up front, but the cat is mostly out of the bag now.  Can we please take discussion about that aspect to a separate bug and stop adding things to this one?

Comment 9

[georgi - hopefully not receiving bugspam]

(In reply to comment #8)
>  Can we please take discussion about that
> aspect to a separate bug and stop adding things to this one?
> 

Bug 379644 

Comment 10

[[e-mail]]

Restricting support to renaming XUL tags would seem to be simple and effective.

Comment 11

[[e-mail]]

If you want restrict it to specific XUL tags then maybe the list in nsDocument::GetBoxObjectFor would be most appropriate.

Comment 12

[Neil Deakin]

(In reply to comment #11)
> If you want restrict it to specific XUL tags then maybe the list in
> nsDocument::GetBoxObjectFor would be most appropriate.
> 

We would also need to allow xul:button and xul:spacer.

Comment 13

[Boris Zbarsky [:bzbarsky]]

One other issue that may arise is people using extends="svg:g" or some such to get around SVG's rules about non-SVG content.  ccing some svg folks who might know whether we care about that sort of thing.

Comment 14

[Robert Longson [:longsonr]]

This sort of thing is discussed in bug 293704 I think.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

We'll see if we can get this in during the Beta 3 bake period, as it would be good to have it for that release, but it's not essential.

Comment 16

[Mike Schroepfer]

We going to get this into B4?

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Nope, leaving as a P1 for now ...

Comment 18

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

I think this could go into an RC. Though I would really prefer to have it in a beta first.

Comment 19

[Johnny Stenback (:jst)]

Surya, Jonas says this would be a good bug for you to work on. He'll be in touch with further details...

Comment 20

[Surya Ismail]

I think the best place to put a white-list check would be here:
http://mxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLService.cpp#818
right where the extends attribute gets parsed.

From comment 2:
> We could also require that this is only done with bindings attached to
> XUL elements.

Does that mean disallow all tag names outside of the xul namespace?  What would a non-XUL element be?

Comment 21

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

(In reply to comment #20)
> I think the best place to put a white-list check would be here:
> http://mxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLService.cpp#818
> right where the extends attribute gets parsed.

Sounds like a good idea.

> Does that mean disallow all tag names outside of the xul namespace?  What 
> would a non-XUL element be?

Not really sure what the question is here?

I think bzs alternative idea was to only allow extends="xul:..." on bindings attached to XUL elements. That way the frame could in fact assume that it was attached to a XUL element.

However I think the simpler solution is to simply have a whitelist of tag names that are allowed to be attached to any element.

Comment 22

[georgi - hopefully not receiving bugspam]

(In reply to comment #21)

> Not really sure what the question is here?
> 
> I think bzs alternative idea was to only allow extends="xul:..." on bindings
> attached to XUL elements. That way the frame could in fact assume that it was
> attached to a XUL element.
> 
> However I think the simpler solution is to simply have a whitelist of tag names
> that are allowed to be attached to any element.
> 

will this whitelist stop the crashes?

what is the proposed whitelist/fix to test it?
(not sure i understood correctly the proposals)

Comment 23

[georgi - hopefully not receiving bugspam]

(In reply to comment #22)

> > I think bzs alternative idea was to only allow extends="xul:..." 

btw, i suspect |extends="html:"| to be safer than extends="xul:"

Comment 24

[Boris Zbarsky [:bzbarsky]]

> btw, i suspect |extends="html:"| to be safer than extends="xul:"

It's actually the other way around, for what it's worth.

Comment 25

[Surya Ismail]

Attachment: Adds white list

Comment 26

[Neil Deakin]

You should just compare nsIAtoms directly rather than converting them to strings.

How did you determine which elements to put in the list? From the list in comment 2? That's actually an arbitrary list.

Comment 27

[Surya Ismail]

(In reply to comment #26)
> You should just compare nsIAtoms directly rather than converting them to
> strings.

Sure, but nsIAtoms doesn't have a case-insensitive string compare.  Would a case sensitive compare be OK?

> How did you determine which elements to put in the list? From the list in
> comment 2? That's actually an arbitrary list.

Searched through the tree, ran it through the automated tests.  That's the best I could think to do.  I'm open to suggestions?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Case-sensitive compare should be fine.  Tag names are case-sensitive.

I'm not sure about the localization stuff in this patch.  We're past l10n freeze.  :(

Comment 29

[Surya Ismail]

(In reply to comment #28)
> I'm not sure about the localization stuff in this patch.  We're past l10n
> freeze.  :(

Yeah, I know, I'm very sorry.  How about taking out the report to console, just have the assertion?  Then you don't need the string?

Comment 30

[Boris Zbarsky [:bzbarsky]]

That might be the way to go for now, or maybe it's worth the late-l10n hit if we think this will cause enough extension-developer pain.  Axel?  Shaver?

Comment 31

[Axel Hecht [:Pike]]

I'd debate localizing XBL error messages in general. Interesting discussion to have for my next stay in a monastry.

Anyway, I'd hardcode the string in the c++ code, and file a bug to get that fixed for mozilla 2, or wherever that piece of code goes out of string freeze.

Comment 32

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

Word on the street is that we don't localize script error messages anyway. Don't know to what extent that is true.

Comment 33

[Mike Shaver (:shaver emeritus)]

We don't localize JS engine errors; we do localize layout-generated errors.  I think this one wouldn't be too bad at all if we did fallback to English in the case of a missing property, and a late-l10n marking to get the locales that have time and cycles.

Comment 34

[Axel Hecht [:Pike]]

That would require me to actually hook up this single entity by hand in two versions of compare-locales. I'd either take it or not, instead of adding more hacks to our process.

Comment 35

[Surya Ismail]

Attachment: Changed tagname compare and report to error console

Comment 36

[Neil Deakin]

(In reply to comment #27)
> (In reply to comment #26)

> > How did you determine which elements to put in the list? From the list in
> > comment 2? That's actually an arbitrary list.
> 
> Searched through the tree, ran it through the automated tests.  That's the best
> I could think to do.  I'm open to suggestions?
> 

I'd remove 'box', 'hbox', 'row' and 'text' from the list, probably 'browser' as well. The first three wouldn't do anything and 'text' is long obsolete.

A more exhaustive list would be the tags checked for in nsDocument::GetBoxObjectFor and in CSSFrameConstructor::ConstructXULFrame but I doubt that most would be necessary.

Comment 37

[Surya Ismail]

(In reply to comment #36)

> I'd remove 'box', 'hbox', 'row' and 'text' from the list, probably 'browser'
> as well. The first three wouldn't do anything and 'text' is long obsolete.

There are elements inside the chrome that extend 'box', 'hbox', 'row' and 'text'. So when I remove them, I get a cascade of assertions.
I can think of two things;
  - go through and modify each instance in chrome where 'box',
    'hbox', 'row' and 'text' are extended
  - check boundDocument and only apply the whitelist to files outside
    of chrome

> A more exhaustive list would be the tags checked for in
> nsDocument::GetBoxObjectFor and in CSSFrameConstructor::ConstructXULFrame but 

When I go through, this is the list I get.

"autorepeatbutton", "browser", "button", "checkbox", "description", "editor", "iframe", "image", "label", "listbox", "listboxbody", "listitem", "menu", "menubar", "menubutton", "menuframe", "menuitem", "menupopup", "panel", "popup", "popupgroup", "progressmeter", "radio", "resizer", "scrollbar", "scrollbarbutton", "scrollbox", "slider", "spacer", "splitter", "spring", "text", "treechildren", "treecol", "titlebar", "tree", "tooltip"

This includes everything in the original list except "box", "hbox" and "row".  I'd vote for the longer list.

Comment 38

[Neil Deakin]

(In reply to comment #37)
> There are elements inside the chrome that extend 'box', 'hbox', 'row' and
> 'text'. So when I remove them, I get a cascade of assertions.


OK, for now, just leave the list as is, and file a bug on investigating which of  the instances in the code are actually necessary.

In the method CheckTagNameWhiteList, I meant you can just make the array of nsIAtoms and compare the atoms directly, using pointer compares rather than using string comparisons. Somewhat like nsIContent::FindAttrValueIn does.

Comment 39

[Surya Ismail]

(In reply to comment #38)

> OK, for now, just leave the list as is, and file a bug on investigating which
> of  the instances in the code are actually necessary.

Ok, will file the bug later today.
 
> Somewhat like nsIContent::FindAttrValueIn does.

Thanks for the code link, now it makes sense to me. Sorry for making you repeat yourself.

Comment 40

[Surya Ismail]

Attachment: Compares with nsIAtom

Comment 41

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

You need to check that the namespace id is kNameSpaceID_XUL in CheckTagNameWhiteList. I'd also make it return a PRBool rather than an nsresult for simplicity.

I also suspect that we want to put svg:g on the whitelist.

Comment 42

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

Comment on attachment 310763 [details] [diff] [review]
Compares with nsIAtom

>+nsresult CheckTagNameWhiteList(PRInt32 aNameSpaceID, nsIAtom *aTagName)
>+{
>+  const nsIAtom* kValidTagNames[] = {
>+    nsGkAtoms::box, nsGkAtoms::browser, nsGkAtoms::button,
>+    nsGkAtoms::hbox, nsGkAtoms::image, nsGkAtoms::menu,
>+    nsGkAtoms::menubar, nsGkAtoms::menuitem, nsGkAtoms::row,
>+    nsGkAtoms::spacer, nsGkAtoms::splitter, nsGkAtoms::text,
>+    nsGkAtoms::tree};

Also, this is a bit scary. Are you sure this will only initialize once, rather than at every call. I suspect it'll actually initialize on every call since there is no guarantee that those variables have the same value. And even if you used 'static const nsIAtom*...' you are relying on a little-known feature of C++ to only initialize this variable the first time the function is called. If it was initialized on library load (which is when static variables are usually initialized) then these variables wouldn't have gotten their correct value. And if it was initialized on every call you'd obviously waste cycles.

So do something like this instead:

  static const nsIAtom** kValidTagNames[] = {
    &nsGkAtoms::box, &nsGkAtoms::browser, &nsGkAtoms::button,
    ...};

  PRUint32 i;
  for (i = 0; i < NS_ARRAY_LENGTH(kValidTagNames); ++i) {
    if (aTagName == *kValidTagNames[i]) {
      ...

It's slightly less performant, but I think it should be fine

Comment 43

[Mike Shaver (:shaver emeritus)]

> And even if you
> used 'static const nsIAtom*...' you are relying on a little-known feature of
> C++ to only initialize this variable the first time the function is called.

Static lifetime function locals and their init-once behaviour are as old as the hills, and well used for these cases -- older than this C++ novelty, for sure! :)

Comment 44

[Surya Ismail]

Attachment: Corrections

Comment 45

[Surya Ismail]

(In reply to comment #42)

> >+nsresult CheckTagNameWhiteList(PRInt32 aNameSpaceID, nsIAtom *aTagName)

static nsIContent::AttrValuesArray seems to be how it's done in other places.
http://mxr.mozilla.org/seamonkey/source/content/svg/content/src/nsSVGAElement.cpp#262

Comment 46

[Surya Ismail]

Filed bug 424746 to investigate bindings that extend box, hbox, text and row.

Comment 47

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

Comment on attachment 311336 [details] [diff] [review]
Corrections

Neil, would be great if you could review this so that we could get this into beta5 to see if it breaks any extensions. Mostly want your review on the list of tag names.


>+  else if (aNameSpaceID == kNameSpaceID_SVG) {
>+    for (i = 0; kValidSVGTagNames[i]; ++i) {
>+      if (aTagName == *(kValidSVGTagNames[i])) {
>+        return PR_TRUE;
>+      }
>+    }
>+  }

I'd just do
else if (aNameSpaceID == kNameSpaceID_SCG &&
         aTagName == nsGkAtoms::g) {
  return PR_TRUE;
}

We can always turn it into a loop later if really needed.

>+            // Check the white list
>+            if (!CheckTagNameWhiteList(nameSpaceID, tagName)) {
>+              nsAutoString errText(NS_LITERAL_STRING("Extending "));
>+              errText.Append(value);
>+              errText.Append(NS_LITERAL_STRING(
>+                          " is invalid. In general, do not extend tag names."));
>+

You can do this as:
nsAutoString errText = NS_LITERAL_STRING("Extending ") + value +
                       NS_LITERAL_STRING(" is invalid....");

That's more efficient and less code.


>Index: content/xbl/test/Makefile.in
> _TEST_FILES =	\
> 		test_bug296375.xul \
> 		test_bug310107.html \
> 		test_bug366770.html \
> 		test_bug371724.xhtml \
> 		test_bug372769.xhtml \
>+        test_bug378518.xul \

Fix indentation by using the same tabbing as the other lines.

Comment 48

[Surya Ismail]

Attachment: Cleaning up code

Comment 49

[Neil Deakin]

Comment on attachment 311533 [details] [diff] [review]
Cleaning up code

The code looks OK, but the test doesn't. That test doesn't actually test what this bug fixes - it will pass even without this fix.

extends="xul:checkbox" doesn't mean extend the checkbox binding, it means use the nsFrame that would have been created for <checkbox>, thus the setChecked function isn't defined anyway.

(As an aside, you should be using 'n1.checked = true' instead of setChecked.)

A better test would be to check if the command event still fires, as this behaviour is done in nsButtonBoxFrame which is applied to checkboxes.

I mentioned in a comment above that menupopup should be in the list.

Comment 50

[Boris Zbarsky [:bzbarsky]]

A good test would be to extends="xhtml:img" and watch the resulting crash?

Comment 51

[Surya Ismail]

Attachment: Add menupopup and revised test

I think the new test is correct. It fails without the change.

Comment 52

[Reed Loden [:reed]]

Checking in content/xbl/src/nsXBLService.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLService.cpp,v  <--  nsXBLService.cpp
new revision: 1.253; previous revision: 1.252
done
Checking in content/xbl/test/Makefile.in;
/cvsroot/mozilla/content/xbl/test/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done
RCS file: /cvsroot/mozilla/content/xbl/test/test_bug378518.xul,v
done
Checking in content/xbl/test/test_bug378518.xul;
/cvsroot/mozilla/content/xbl/test/test_bug378518.xul,v  <--  test_bug378518.xul
initial revision: 1.1
done

Comment 53

[Reed Loden [:reed]]

Backed out as the possible cause for a ton of test failures.

Comment 54

[Surya Ismail]

(In reply to comment #53)

Hi Reed,
Sorry for the test failures. I'll re-run the tests and expand the white list. Hopefully that fixes things. Are there any tests in particular I should look at?

Comment 55

[Neil Deakin]

Looks like you will also need xul:slider. Not sure what is wrong with the numberbox test. Seems like something to do with the spinbuttons.

Comment 56

[Surya Ismail]

Would it make sense, at this point, to just add all the tags from nsDocument::GetBoxObjectFor and CSSFrameConstructor::ConstructXULFrame?

Comment 57

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

We definitely don't want all the tags from CSSFrameConstructor::ConstructXULFrame. That would completely defeat the purpose of the patch since what we're trying to prevent are the entries in CSSFrameConstructor::ConstructXULFrame that aren't safe.

Comment 58

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

Surya: Any updates here? Do you need any help or are you on it?

Comment 59

[Surya Ismail]

Sorry for the delay, been having trouble with my build environment since the requirement for the Windows Vista SDK. Should get it resolved by tomorrow, then will run the tests. The fix shouldn't take long.

Comment 60

[Mike Schroepfer]

Going to take this off the blocker list for now since we are closing down for RC1.  Surya please keep working on the patch - when it is ready we'll evaluate risk/reward and the current state of the release to determine if this goes into RC or into a 0.x.

Comment 61

[Robert Longson [:longsonr]]

Comment on attachment 312799 [details] [diff] [review]
Add menupopup and revised test

>+PRBool CheckTagNameWhiteList(PRInt32 aNameSpaceID, nsIAtom *aTagName)
>+{
...
>+  else if (aNameSpaceID == kNameSpaceID_SVG) {
>+    if (aTagName == nsGkAtoms::g) {
>+      return PR_TRUE;
>+    }
>+  }

Can this be removed? We really don't want to allow mucking about with SVG frames. 

In the unlikely event that this does allow you to construct an svg <g> frame without some kind of svg element content the SVG code will almost certainly crash. We have tried to put checks in the SVG frame construction code to check that it only works for SVG content.

Comment 62

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

Definitely sounds like it should be removed then as those crashes is the whole point of this bug :)

This does mean that you can't really add custom elements inside SVG and style them using XBL though. Was that intended? I.e. even if the custom element lives inside a SVG document, and the binding only contains SVG elements, thing will not work.

Comment 63

[Robert Longson [:longsonr]]

If you have an nsSVGGFrame then GetContent() on that better give you an nsSVGGElement (or something derived from that). 

You can't have custom elements wrapped by SVG frames as they won't support the right interfaces (we cast everywhere to the element we "know" it should be) and we try as best we can when constructing the frames to ensure that "know" works.

Maybe if someone who knew xbl audited the SVG layout code, presumably post 1.9 they could fix it.

Comment 64

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

We only need to audit the frames that we whitelist in this patch, but yeah, if even the svg:g frame makes that assumption then we can't whitelist it yet.

Weird though, I know the original SVG implementation used XBL in a bunch of demos and it didn't seem to crash back then...

Comment 65

[Robert Longson [:longsonr]]

Presumably it depends what you are trying to do with SVG. There are demos at http://croczilla.com/svg/samples/ which seem OK as they use SVG elements to get SVG frames which they then manipulate using xbl.

Comment 66

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

(In reply to comment #65)
> Presumably it depends what you are trying to do with SVG. There are demos at
> http://croczilla.com/svg/samples/ which seem OK as they use SVG elements to get
> SVG frames which they then manipulate using xbl.

No, it doesn't use SVG elements. All the elements in the main document are in the null namespace and so will not inherit nsSVGElement. But they still currently get svg frames since the XBL says inherits="svg:generic".

What you are proposing will make that testcase *not* work.

We currently create an nsSVGGenericContainerFrame for such nodes. Are you sure that such frames are not safe to use on non-svg elements?

Comment 67

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

So I looked at the code for the nsSVGGenericContainerFrame class and it looks safe to me. In fact, it doesn't look like it does anything with it's content node at all, other than in code it inherits from nsContainerFrame.

So I think we should put "svg:generic" on the whitelist.

Comment 68

[Robert Longson [:longsonr]]

(In reply to comment #66)
> We currently create an nsSVGGenericContainerFrame for such nodes. Are you sure
> that such frames are not safe to use on non-svg elements?
> 

That's fine. I thought we were creating nsSVGGFrames (based on the nsGkAtoms::g tag in the patch) which would have been a problem.

(In reply to comment #67)
> So I think we should put "svg:generic" on the whitelist.
> 

OK.

Comment 69

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

(In reply to comment #68)
> That's fine. I thought we were creating nsSVGGFrames (based on the nsGkAtoms::g
> tag in the patch) which would have been a problem.

Yup, you're right, my initial proposal was to put svg:g on the whitelist which wouldn't have been safe enough.

Comment 70

[Boris Zbarsky [:bzbarsky]]

Actually, it would be fine.  As Robert pointed out, NS_NewSVGGFrame would return null if not passed an SVGElement, and we'd fall into the same codepath as svg:generic.

Comment 71

[Surya Ismail]

Attachment: More whitelist items

This seems to pass all automated tests. Added xul:slider and xul:autorepeatbutton and svg:generic.

Comment 72

[Surya Ismail]

Attachment: Fixed bad return value

Should this go into RC2? I've run it through automated testing twice now, so I think the risk is pretty low.

Comment 73

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

Ugh, sorry, didn't notice that this was waiting for my review.

Anyway, I think this is too risky for RC2, we should land it on trunk as soon as it opens, and then land on branch after some baking.

Comment 74

[Surya Ismail]

(In reply to comment #73)

> Anyway, I think this is too risky for RC2, we should land it on trunk as soon
> as it opens, and then land on branch after some baking.

Ok, sounds good. Thanks.

Comment 75

[Neil Deakin]

Since we aren't putting this in 3.0 any more, we could add back the localization and just use ReportToConsole.

Comment 76

[Surya Ismail]

Attachment: Changed error reporting

Comment 77

[Neil Deakin]

Comment on attachment 325190 [details] [diff] [review]
Changed error reporting

>+              nsContentUtils::ReportToConsole(nsContentUtils::eXBL_PROPERTIES,
>+                                              "InvalidExtendsBinding",
>+                                              params, NS_ARRAY_LENGTH(params),
>+                                              boundDocument->GetDocumentURI(),

Looks good, but is this right uri? From the name 'boundDocument' seems like the bound xul document, whereas it seems like we'd want to report the xbl document's uri.

Comment 78

[Surya Ismail]

Attachment: Changed doc for error reporting

(In reply to comment #77)
Good point about the boundDocument. Sorry I missed it. Have changed it.

Comment 79

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

Comment on attachment 325495 [details] [diff] [review]
Changed doc for error reporting

>+PRBool CheckTagNameWhiteList(PRInt32 aNameSpaceID, nsIAtom *aTagName)
>+{
>+  static nsIContent::AttrValuesArray kValidXULTagNames[] =  {
>+    &nsGkAtoms::autorepeatbutton, &nsGkAtoms::box, &nsGkAtoms::browser,
>+    &nsGkAtoms::button, &nsGkAtoms::hbox, &nsGkAtoms::image, &nsGkAtoms::menu,
>+    &nsGkAtoms::menubar, &nsGkAtoms::menuitem, &nsGkAtoms::menupopup,
>+    &nsGkAtoms::row, &nsGkAtoms::slider, &nsGkAtoms::spacer,
>+    &nsGkAtoms::splitter, &nsGkAtoms::text, &nsGkAtoms::tree, nsnull};
>+
>+  PRUint32 i;
>+  if (aNameSpaceID == kNameSpaceID_XUL) {
>+    for (i = 0; kValidXULTagNames[i]; ++i) {
>+      if (aTagName == *(kValidXULTagNames[i])) {
>+        return PR_TRUE;
>+      }
>+    }
>+  }
>+  else if (aNameSpaceID == kNameSpaceID_SVG) {
>+    if (aTagName == nsGkAtoms::generic) {
>       return PR_TRUE;

Nit: I would just write the last one as

else if (aNameSpaceID == kNameSpaceID_SVG &&
         aTagName == nsGkAtoms::generic) {
  return PR_TRUE;  
...

Or even change the final return to:

return aNameSpaceID == kNameSpaceID_SVG &&
       aTagName == nsGkAtoms::generic;

Looks great otherwise!

Comment 80

[Surya Ismail]

Attachment: Fixed review comment

Comment 81

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central (attachment 329519 [details] [diff] [review]):
http://hg.mozilla.org/mozilla-central/index.cgi/rev/e888893e48ba