541506 - Crash with userstyle for addon Stylish on golem.de [@ nsTextControlFrame::SetInitialChildList(nsIAtom*, nsFrameList&) ]

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect)

Description

[ToBo]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2) Gecko/20100115 Firefox/3.6

With the new FF 3.6 Final i get a crash on Firefox, if i browse to adress www. golem.de. On previous FF Version 3.5 i never had a problem with this.
 
I identify the addon Stylish and a specific userstyle http://userstyles.org/styles/10704 for the website www.golem.de. In this code i found that the last code block
 
 /**
  * Remove ad at the top of the menu
  * (maybe to specific)
  */

  td.headerbg div, td.headerbg h5[style="text-transform: none; text-align: center; background-color: rgb(184, 23, 26);"] {
  display: none !important;
 }
 
the malfactor for the FF crash. If i delete this code block i get no crash.

Here is the complete userstyle:
@namespace url(http://www.w3.org/1999/xhtml);



@-moz-document domain("golem.de") {



/**

 * User style for geman language news site www.golem.de.

 *

 * http://userstyles.org/styles/10704

 *

 * Feel free to make suggestions and report bugs.

 */



 /**

  * Full width for homepage, articles and forum

  */

 table[width="972"] {

  width: 100% !important;

 }



 /**

  * Full width for screenshot overviews

  */

 table[width="932"] {

  width: 100% !important;

 }



 /**

  * Full width for tables with subheaders

  */

 table[width="480"] {

  width: 100% !important;

 }



 /**

  * Reduce page height to visible content

  */

 td.leftedge img {

  height: 100% !important;

 }



 /**

  * Left alignment for newsletter input field 

  */

 td.headerbg form table[align="right"] {

  align: left !important;

 }



 /**

  * Fixed width for right column in forum pages

  *

  * Thanks to DreadKing and ChoGGi!

  * (http://forum.userstyles.org/comments.php?DiscussionID=2560)

  */

 table[width="972"]>tbody>tr>td[valign="top"][rowspan="3"] {

  width:280px!important

 }



 /**

  * Remove header ad

  */

 #big {

  display: none !important

 }



 /**

  * Remove skyscraper ad

  */

 table td[rowspan="5"] {

  width: 0 !important;

 }



 /**

  * Remove skyscraper ad

  */

 table td[rowspan="5"] #sky {

  display: none !important; 

 }



 /**

  * Remove ad in the article

  */

 #contentad {

  display: none !important;

 }



 /**

  * Remove ad in the article

  */

 #contentad2 {

  display: none !important;

 }



 /**

  * Remove footer ad

  */

 div[style="margin-top: 10px; clear: both;"] {

  display: none !important

 }



 /**

  * Remove ad at the top of the menu

  * (maybe to specific)

  */

 td.headerbg div, td.headerbg h5[style="text-transform: none; text-align: center; background-color: rgb(184, 23, 26);"] {

  display: none !important;

 }



}

Reproducible: Always

Steps to Reproduce:
1. Install addon stylish(https://addons.mozilla.org/de/firefox/addon/2108) on FF 3.6
2. Install userstyle http://userstyles.org/styles/10704 and activate the userstyle
3. browse to www.golem.de
4. FF crash
Actual Results:  
FF crash

Expected Results:  
FF get a error or works fine as FF 3.5, but no crash.

FF should not crash

Comment 1

[Robert Longson [:longsonr]]

You should report this to the author of stylish

Comment 2

[ToBo]

(In reply to comment #1)
> You should report this to the author of stylish

Yes, i report it to author, but i think it's a FF Bug.

Comment 3

[Robert Longson [:longsonr]]

> (In reply to comment #1)
> > You should report this to the author of stylish
> 
> Yes, i report it to author, but i think it's a FF Bug.

What method is the add on calling when it crashes? What arguments is it passing? The add on author can tell us and raise a bug if required with sufficient information that we may be able to fix it.

Comment 4

[Dave Garrett]

Easily reproduced using a new profile in Firefox 3.6 on Linux with given STR. Not invalid; confirming.

bp-c1bd0007-27f9-470e-ab18-1e2c32100122
0  	libxul.so  	nsTextControlFrame::SetInitialChildList  	 layout/generic/nsQueryFrame.h:278
1 	libxul.so 	nsCSSFrameConstructor::ConstructFrameFromItemInternal 	layout/base/nsCSSFrameConstructor.cpp:4069
2 	libxul.so 	nsCSSFrameConstructor::ConstructFramesFromItem 	layout/base/nsCSSFrameConstructor.cpp:5644
3 	libxul.so 	nsCSSFrameConstructor::ConstructFramesFromItemList 	layout/base/nsCSSFrameConstructor.cpp:9609
4 	libxul.so 	nsCSSFrameConstructor::ProcessChildren 	layout/base/nsCSSFrameConstructor.cpp:9717
5 	libxul.so 	nsCSSFrameConstructor::ConstructTableCell 	layout/base/nsCSSFrameConstructor.cpp:2363
...
...

For future reference for crashes, post crash IDs:
https://developer.mozilla.org/en/How_to_get_a_stacktrace_for_a_bug_report

Comment 5

[Dave Garrett]

The crashing user style can be further reduced to simply:

@namespace url(http://www.w3.org/1999/xhtml);
@-moz-document domain("golem.de") {
 td.headerbg div {
  display: none;
 }
}

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase (uses enhanced privileges)

This is also crashing trunk builds.

I have also a case with a similar testcase, that crashes on print preview.

It seems bad to me that Stylish is also modifying anonymous content. I don't think that is something they want. But perhaps there is no better way of doing this?

Comment 7

[Alice0775 White]

It is well known issue
https://developer.mozilla.org/en/Using_the_Stylesheet_Service#Using_the_API

Comment 8

[Martijn Wargers (dead)]

Yeah, I can vaguely remember some discussion about that (on bugzilla?).
Anyway, shouldn't there be an API there that does the same as AGENT_SHEET, but doesn't count for native anonymous elements?

Comment 9

[Daniel Veditz [:dveditz]]

Don't see how adding an API can block a branch release, especially when it's not even assigned to someone to fix on trunk.

Comment 10

[Martijn Wargers (dead)]

Me neither, but I don't see a blocking1.9.3? flag.

Comment 11

[Dave Garrett]

Gecko 1.9.3 became 2.0 => the flag is blocking2.0 now

Comment 12

[Martijn Wargers (dead)]

Confusing, I thought that flag was for fennec2.0.

Comment 13

[Dave Garrett]

Fennec is handled in the blocking-fennec flag. Gecko blocking flag info:
https://wiki.mozilla.org/Releases/Flags

Comment 14

[Martijn Wargers (dead)]

Yes, I know now, but it is still confusing. Especially since the 2.0 flag is just under the fennec1.1 flag.

Comment 15

[Martijn Wargers (dead)]

It seems like the fennec1.1 flag has now changed into the fennec flag.

Comment 16

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

Stylish should be using USER_SHEET -- that's what it's for.  I agree that this is a bug in Stylish, so minusing on that basis.

Comment 17

[ToBo]

The crash is gone, if i use my Firefox4 beta4, no crash here.

Comment 18

[Alice0775 White]

The browser crashes. when the following CSS is added into stylish 1.0.x.
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100824 Minefield/4.0b5pre ID:20100824040950

@namespace url(http://www.w3.org/1999/xhtml);
div {overflow:visible !important;}


This CSS hits https://developer.mozilla.org/en/Using_the_Stylesheet_Service#Using_the_API .

Comment 19

[Martijn Wargers (dead)]

(In reply to comment #6)
> I have also a case with a similar testcase, that crashes on print preview.

After minimizing that testcase, I got a different stacktrace, so I filed a new bug for it, bug 590302.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Jorge, could we get Stylish to stop creating UA sheets with broken styling in them?  UA sheets are allowed to violate various layout invariants (not least because we use UA sheets to implement them to start with), so can easily cause crashes or other sorts of bad behavior.

Comment 22

[Jorge Villalobos [:jorgev] (he/him)]

Adding Jason Barnabe, since I believe he can answer this better. Stylish is like Greasemonkey, in that it will run user-generated styles, some of which may be problematic. It's probably very difficult to implement a filter for these broken styles, but I'm interested in the suggestion given in comment #16, and would like to know why it isn't used in current versions of Stylish.

Comment 24

[Jason Barnabe (np)]

Stylish used USER_SHEET until bug 369676 went in (see also bug 424184), at which time I switched it to AGENT_SHEET. There were and still are a number of styles on userstyles.org which changed how scrollbars work, and not making the switch would have made those styles non-functional.

It's a known issue (by me, at least) that you can cause crashes with Stylish if your selectors match anonymous content and you change certain properties (especially "overflow"). It's also been made clear to me that the crashes caused by such styles will not be fixed by the Mozilla devs (see bug 491067). It's a trade-off I've accepted.

Comment 25

[Boris Zbarsky [:bzbarsky]]

It's not "will not" but "cannot, without introducing yet another level of even more privileged stylesheets".  Except we'd have to restrict scrollbar styling to _that_ level, then, and then Stylish would switch to using that level or something.

Maybe what we should do is just disallow extensions adding UA-level sheets and be done with it.  :(

Comment 26

[Andreas Eibach]

Well, I'm the submitter of bug 925984. Before making any hasty decisions and/or assumptions, can you guys please check out the gdb backtrace (using a Firefox DEBUG build with symbols) which I provided? Maybe it will give more insight...

Comment 27

[Andreas Eibach]

After some more testing, it's evident that Jason and Martijn are actually hitting spot-on here. In my case it was #something DIV {...} in CSS. It's indeed anonymous elements, in particular _anonymous DIVs_ that provoke this. Once you assign the DIV a class name, this madness stops. However, for user styles, it must be noted that we're doomed to what the *website author* has specified: whenever they chose to use an _anonymous_ div instead of a <"div id="foobar_div">, we cannot just invent an artificial temp class name just to prevent this crash. In fact we "user-stylers" will either have to count on the site author's goodwill so that he/she will make their site "Stylish-compatible". If not, the only (very ugly) way out will be via a parent element that _does_ have a proper DOM ID.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Andreas, I did look at your stack.  The point is that your style rearranges the insides of text inputs in a way they don't expect, causing them to crash.  This has nothing to do with the website author and everything to do with the fact that Stylish purposefully applies your styles to layout implementation details like the insides of scrollbars and the insides of text inputs.  Once you do that, if you use the wrong style all bets are off.

Jason, could Stylish have two buckets of stylesheets: ones meant to style scrollbars and ones not trying to do it?  And then only use AGENT_SHEET for the former?

Comment 29

[Andreas Eibach]

Boris, thanks for taking a look into it. But the main problem I see is that there is no guideline what we may do and what we may not do. Nowhere. When applying a custom style to a web site, how should we know where to keep our fingers off (i. e. what _are_ the actual invariants?) and where it's safe to mess around? It's on you guys to know how the CSS parser internals work - _we_ can't do but some arbitrary guesswork.

Comment 30

[Jason Barnabe (np)]

Andreas, I think you may be misunderstanding the concept of anonymous content. An "anonymous div" is not merely a div with no ID or class attribute, but rather it's part of Firefox's internal representation of an element. For example, if you use <input type="file">, Firefox internally translates this to a label, a text input, and a button. This is the anonymous content. When you use "#something div" as a selector in this case, you're actually matching part of this anonymous content, and if you change it ways that Firefox doesn't expect, it can cause crashes.

There are sometimes ways to change your selectors so they don't match the anonymous content. For example, if all the divs you want to affect are direct children of #something, you could use "#something > div" instead. Alternately, as you mention, if there are IDs, classes, or attributes on these divs, you could select on that.

Boris, I've given some thought to switching how styles are registered. You can see the pros and cons each registration type from my point of view at https://github.com/JasonBarnabe/stylish/issues/78#issuecomment-25424269 . I will do some investigation of styles posted on userstyles.org to see if there's any code patterns that indicate a scrollbar style (or other intentionally anonymous-content-affecting style). This may allow me to make Stylish  register styles as USER or AUTHOR, limiting AGENT to when that pattern is detected.

I don't see this as a bug in Stylish. Given that Mozilla doesn't want to guard against these kinds of crashes (a decision I accept), I see it as conscious decision on my part to accept the possibility of crashes in exchange for the possibility of styling the internals, like scrollbars. There are a myriad of other ways you can seriously bork Firefox with Stylish without crashing it, anyway.

Comment 31

[Andreas Eibach]

>There are sometimes ways to change your selectors so they don't match the anonymous content. For >example, if all the divs you want to affect are direct children of #something, you could use >"#something > div" instead.

Ah, thanks Jason!! That "fixed" the crash. So it must been some greater ambiguity I created, i. e. the engine didn't know what decision to take. BTW, my actual style is 500+ lines, but I'm against posting pagesfuls of style code, which is considered 'rude' here.

Again, in a nutshell;

#something DIV {
...
}
---> CRASH

#something > DIV {
...
} 
---> NO CRASH

Great!

Comment 32

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Jason Barnabe (np) from comment #30)
> Boris, I've given some thought to switching how styles are registered. You
> can see the pros and cons each registration type from my point of view at
> https://github.com/JasonBarnabe/stylish/issues/78#issuecomment-25424269 . I
> will do some investigation of styles posted on userstyles.org to see if
> there's any code patterns that indicate a scrollbar style (or other
> intentionally anonymous-content-affecting style). This may allow me to make
> Stylish  register styles as USER or AUTHOR, limiting AGENT to when that
> pattern is detected.

Would it be possible to move to a system where userstyle developers have to opt-in to the AGENT_SHEET behavior? This would break some of the current userstyles, but that could be minimized by notifying developers about the change and making a slow transition.

Comment 33

[Jason Barnabe (np)]

That's definitely a possibility. I'd prefer the switch to AGENT_SHEET to be automatic based on the code content if possible, because it reduces the requirements on style authors' part. Many of them only have basic CSS knowledge (they may simply be flipping colors around on an existing style) and English proficiency. You'd end up with styles that should flip the switch not flipping it and vice versa.

Comment 34

[Boris Zbarsky [:bzbarsky]]

> I will do some investigation of styles posted on userstyles.org to see if there's any
> code patterns that indicate a scrollbar style (or other intentionally
> anonymous-content-affecting style)

That would be awesome.  Thank you!

Comment 35

[Jason Barnabe (np)]

Stylish 1.4.1b1 now uses AUTHOR by default, so giving it this code no longer crashes:

div {overflow:visible !important;}

Authors can opt-in to AGENT with the magic comment /* AGENT_SHEET */, with the potential crashiness that entails. Styles posted on userstyles.org that appear to be for scrollbars have had this added automatically.

http://forum.userstyles.org/discussion/39738/stylish-for-firefox-1-4-1b1
https://github.com/JasonBarnabe/stylish/wiki/Overwriting-page-styles

Comment 36

[Boris Zbarsky [:bzbarsky]]

Jason, thank you for doing that!

Comment 37

[Andreas Eibach]

I second that! Thanks for your hard work on that not-too-trivial issue.

Comment 38

[Jason Barnabe (np)]

Stylish 1.4.1 is now available on AMO. https://addons.mozilla.org/en-US/firefox/addon/stylish/

Comment 39

[Mats Palmgren (inactive)]

If you override !important declarations from the UA sheet you're
definitely well into "undefined behavior" territory.  Don't do that.
I don't think that's something we want to support at all.

It seems like most of the crashes here are gone now though, presumably
due to making AGENT the default in Stylish, so that's good.