564063 - Crash [@ nsCSSFrameConstructor::MaybeConstructLazily]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase causes an assertion failure and a crash.

###!!! ASSERTION: Ancestors of nodes with frames to be constructed lazily should have frames: 'content->GetPrimaryFrame() || (content->GetFlattenedTreeParent() && content->GetFlattenedTreeParent()->GetPrimaryFrame() && content->GetFlattenedTreeParent()->GetPrimaryFrame()->IsLeaf())', file /Users/jruderman/mozilla-central/layout/base/nsCSSFrameConstructor.cpp, line 6240

Crash [@ nsCSSFrameConstructor::MaybeConstructLazily] trying to evaluate the condition of the following assertion.  So much for modifying the assertion being a "safe fix" (bug 560441 comment 3) ;)

I don't know why I need a timer in this testcase.

Comment 1

[Timothy Nikkel (:tnikkel)]

The setTimeout is needed because, wait for it, area's suck: they don't setup their primary frame to be the image until "later", fun stuff.

Comment 2

[Jesse Ruderman]

(tn says the following methods force it to hook up immediately: force the associated image to paint using drawwindow, dispatch a synth mouse event to the associated image, or dynamically modify the dom under the map element)

Comment 3

[Timothy Nikkel (:tnikkel)]

The first two require enhanced privileges, and the third works if you insert or remove anything, even a textnode, directly as a child of the map element.

Comment 4

[Timothy Nikkel (:tnikkel)]

We insert to an area, it already has a primary frame (the image), its parent content is the map, which doesn't have a frame. Things are complicated further because the map has just been inserted after being removed (removing the map element from the document doesn't affect the image map because the image map mutation observers don't catch this case, see bug 135040 comment 10), so the map also has the NEEDS_FRAME bit set.

I think we just need to detect when a node has a "bogus" primary frame like area's do, and be more lax in the assertion on the remaining ancestor nodes. This is going to make the asserts the most complicated part of that function, but what else can you do?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fix bug 135040? *runs away*

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er, with s/area's/areas/

Comment 8

[Timothy Nikkel (:tnikkel)]

Landed
http://hg.mozilla.org/mozilla-central/rev/0dac5d3eeb97

but backed out because something in the push was causing orange
http://hg.mozilla.org/mozilla-central/rev/01befa5163ee

Comment 9

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/d16b9e976729

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to comment #2)
> (tn says the following methods force it to hook up immediately: force the
> associated image to paint using drawwindow, dispatch a synth mouse event to the
> associated image, or dynamically modify the dom under the map element)

Upon further thought, the third method will only work to get the image to update to changes made in a map for which the image is already hooked up to. To get an image to initially hook up to a map you need to use one of the first two methods.