1149932 - Browser crashes when attaching CSS styles

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: pixelperfect-2.0.2.xpi

Crash report:
https://crash-stats.mozilla.com/report/index/2265eda4-75c2-43be-b2a9-0f9b32150331

(not sure if Layout is the right component)

STR:
0. Load any page e.g. google.com 
1. Install attached extension (Pixel Perfect, see docs: https://github.com/firebug/pixel-perfect/wiki)
2. You should see a new button in the main Firefox toolbar
3. Click the button and create a new layer (as describe in the popup windows)
4. Refresh the page, the browser crashes -> BUG

It's related to how CSS styles are attached to anonymous content, see:
https://github.com/firebug/pixel-perfect/blob/pixelperfect-2.0.2/lib/pixel-perfect-actor.js#L533

If you disable this line:

attach(style, win);

the crash is gone.

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Also note that it's reproducible in Firefox 36 and 37.
I don't see the crash in Firefox 38

Honza

Comment 2

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Looks like we're crashing in this loop in nsCSSFrameConstructor::RecreateFramesForContent:

       nsIAnonymousContentCreator* acc = nullptr;
       nsIFrame* ancestor = frame->GetParent();
       while (!(acc = do_QueryFrame(ancestor))) {
         ancestor = ancestor->GetParent();
       }

in the frame->GetStateBits() & NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT case.

On trunk, this loop uses nsLayoutUtils::GetParentOrPlaceholderFor instead of GetParent().  That change was made in bug 1116714, which was in fact fixed in 38.  The commit message for that changeset is:

  Bug 1116714 part 3 - Use GetParentOrPlaceholderFor (not GetParent) so that reframing
  anonymous content frames works also for fixed pos frames.  r=roc

Are you attaching fixed-position anonymous content, by any chance?

Mats, does it make sense to backport that patch?  I guess at this point we'd have to do a chemspill for it, so it might not be worth it...

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

> Are you attaching fixed-position anonymous content, by any chance?
Not fixed but absolute.

div:-moz-native-anonymous.moz-custom-content-container {
  position: absolute;
}

You can see the stylesheet here:
https://github.com/firebug/pixel-perfect/blob/pixelperfect-2.0.2/chrome/skin/classic/shared/ua.css

Honza

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, that would have the same effect, I expect.

Comment 5

[Mats Palmgren (inactive)]

(In reply to Not doing reviews right now from comment #2)
> Mats, does it make sense to backport that patch?

It would be very low risk to backport that patch only, if that helps.

> I guess at this point we'd
> have to do a chemspill for it, so it might not be worth it...

Yeah, this doesn't seem chemspill worthy to me.

Comment 6

[Mats Palmgren (inactive)]

It might be possible to avoid the bug by never doing any style
changes that will recreate the CSS box.  E.g. remove the element
from the document, change the style, insert it again.
Not sure how feasible that is in this case.

Comment 7

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm tracking 37 so that we can consider this as a ride along for a point release. Unfortunately we missed the 37.0.1 release that will ship today. I'm not tracking for 38 as it is marked as unaffected.

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

Firefox 37 will be EOL on Tuesday. I don't think it's worth following up on this. I'm resolving this bug as wontfix. Note that this issue is not present in Firefox 38 and later.