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

NEW
Unassigned

Status

()

Core
CSS Parsing and Computation
--
critical
7 years ago
2 years ago

People

(Reporter: ToBo, Unassigned)

Tracking

({crash, testcase})

Trunk
x86
All
crash, testcase
Points:
---

Firefox Tracking Flags

(blocking2.0 -, blocking1.9.2 -)

Details

(Whiteboard: [bug in Stylish addon], crash signature, URL)

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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

7 years ago
You should report this to the author of stylish
Status: UNCONFIRMED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INVALID
(Reporter)

Comment 2

7 years ago
(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

7 years ago
> (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

7 years ago
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
Severity: normal → critical
Status: RESOLVED → UNCONFIRMED
OS: Windows 7 → All
Resolution: INVALID → ---
Summary: Crash with userstyle for addon Stylish → Crash with userstyle for addon Stylish on golem.de [@ nsTextControlFrame::SetInitialChildList(nsIAtom*, nsFrameList&) ]
Version: unspecified → 3.6 Branch

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 5

7 years ago
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;
 }
}
Component: General → Layout
Keywords: crash
Product: Firefox → Core
QA Contact: general → layout
Version: 3.6 Branch → 1.9.2 Branch
Created attachment 455884 [details]
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?

Updated

7 years ago
Keywords: testcase
Version: 1.9.2 Branch → Trunk

Comment 7

7 years ago
It is well known issue
https://developer.mozilla.org/en/Using_the_Stylesheet_Service#Using_the_API
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?
blocking1.9.2: --- → ?
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.
blocking1.9.2: ? → -
Me neither, but I don't see a blocking1.9.3? flag.

Comment 11

7 years ago
Gecko 1.9.3 became 2.0 => the flag is blocking2.0 now
Confusing, I thought that flag was for fennec2.0.
blocking2.0: --- → ?

Comment 13

7 years ago
Fennec is handled in the blocking-fennec flag. Gecko blocking flag info:
https://wiki.mozilla.org/Releases/Flags
Yes, I know now, but it is still confusing. Especially since the 2.0 flag is just under the fennec1.1 flag.
It seems like the fennec1.1 flag has now changed into the fennec flag.
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.
blocking2.0: ? → -
Component: Layout → Style System (CSS)
QA Contact: layout → style-system
(Reporter)

Comment 17

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

Comment 18

7 years ago
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 .
(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.
(Assignee)

Updated

6 years ago
Crash Signature: [@ nsTextControlFrame::SetInitialChildList(nsIAtom*, nsFrameList&) ]
Duplicate of this bug: 925984
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.
Flags: needinfo?(jorge)
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.
Flags: needinfo?(jorge) → needinfo?(jason.barnabe)

Updated

4 years ago
Duplicate of this bug: 925984

Comment 24

4 years ago
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.
Flags: needinfo?(jason.barnabe)
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.  :(
Whiteboard: [bug in Stylish addon]

Comment 26

4 years ago
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

4 years ago
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.
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

4 years ago
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

4 years ago
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

4 years ago
>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!
(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

4 years ago
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.
> 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

3 years ago
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
Jason, thank you for doing that!

Comment 37

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

Comment 38

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

Updated

2 years ago
Crash Signature: [@ nsTextControlFrame::SetInitialChildList(nsIAtom*, nsFrameList&) ] → [@ nsTextControlFrame::SetInitialChildList(nsIAtom*, nsFrameList&) ] [@ nsTextControlFrame::SetInitialChildList ]
You need to log in before you can comment on or make changes to this bug.