Closed Bug 421203 Opened 16 years ago Closed 16 years ago

Crash [@ InlineBackgroundData::GetContinuousRect] with XUL, rtl, background image

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: jruderman, Assigned: dbaron)

References

Details

(7 keywords)

Crash Data

Attachments

(6 files, 5 obsolete files)

6.93 KB, text/plain
Details
3.02 KB, application/vnd.mozilla.xul+xml
Details
1.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
20.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
83.23 KB, image/png
Details
4.80 KB, patch
roc
: review+
Details | Diff | Splinter Review
Attached file testcase (crashes Firefox when loaded) (obsolete) —
Loading the testcase triggers an assertion that was added in bug 412093:

###!!! ASSERTION: Cannot find containing block.: 'NS_SUCCEEDED(rv) && mBlockFrame', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSRendering.cpp, line 267

followed by a crash [@ InlineBackgroundData::GetContinuousRect].

The testcase seems to depend on the image being loaded remotely; replacing the image URL with a data: URL makes the crash go away.
Blocks: 412093
Attached file stack trace
When we hit the assertion |frame| is not null, but isn't a block frame either. Rather, it's a nsDocElementBoxFrame.

This could be easily patched up in a couple of ways (put this block in mBlockFrame even though it isn't one; just assume LTR if mBlockFrame is NULL), but I'm not sure either of these is right.
I'm also not really sure where I got that code for finding the containing block.

Roc - could you suggest a better way for finding the (block) element that establishes the base direction for bidi purposes - preferably one that always returns some result?
What does the frame tree look like in that bug? It sounds like it's a bad frame tree. Inline frames should always be wrapped in blocks somewhere.
Frame dump:

Viewport(-1)@0xc5c0ac [view=0x17263210] {0,0,36600,22860} [state=00042020] [content=0x0] [sc=0xc5bfb0] pst=:-moz-viewport<
  RootBox(window)(-1)@0xc5c140 {0,0,36600,22860} [state=81c50000] [content=0x1d9d08b0] [sc=0xc5c1b4] pst=:-moz-canvas<
    DocElementBox(window)(-1)@0xc5c47c {0,0,36600,22860} [state=81a50000] [content=0x1d9d08b0] [sc=0xc5c2b0]<
      PopupSet(popupgroup)(-1)@0xc5c578 next=0xc5c8f8 {0,0,36600,0} [state=80c50000] [content=0x1d5dc560] [sc=0xc5c3c4]<
        Popup-list for PopupSet(popupgroup)(-1)@0xc5c578 <
          MenuPopup(tooltip)(-1)@0xc5c7cc [view=0x172e0ad0] {0,0,0,0} [state=80812502] [content=0x1721c5d0] [sc=0xc5c714]<>
        >
      >
      Placeholder(tooltip)(-1)@0xc5c8f8 {0,0,36600,0} outOfFlowFrame=MenuPopup(tooltip)(-1)@0xc5c7cc
      Inline(hbox)(0)@0xc5c9d0 {0,0,36600,22860} [content=0x1d584580] [sc=0xc5c634]<>
    >
  >
>
Yeah, that frame tree is bogus, the inline should not be a child of the DocElementBox. Not your bug.
Component: Layout: BiDi Hebrew & Arabic → Layout
QA Contact: layout.bidi → layout
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
tree.gif is gone :(
Attachment #307612 - Attachment is obsolete: true
Same happens on Vista. Here the crash id: bp-f41d6b17-ad30-11dd-a2ae-001cc4e2bf68
OS: Mac OS X → All
Hardware: PC → All
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P3
I think the problem here is that bug 321402 didn't patch ConstructDocElementFrame, but it should have.
I think we should just move this into ProcessChildren.  (The other potentially problematic caller is nsCSSFrameConstructor::LazyGenerateChildrenEvent::Run.)
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
This passes reftests, crashtests, chrome mochitests, and browser-chrome mochitests.

This patch changes how we handle text children of the root element in XUL:  we now start displaying them ,like we did for bug 321402 for non-root XUL elements.  Oddly enough, the only tests depending on the broken behavior were three of the tests for bug 321402 itself, which had some broken markup in them.
Attachment #352802 - Attachment is obsolete: true
Attachment #352813 - Flags: superreview?(roc)
Attachment #352813 - Flags: review?(roc)
> This patch changes how we handle text children of the root element in XUL: 
> we now start displaying them

Please add a reftest reflecting this intentional behavior change :)
Attachment #352813 - Flags: superreview?(roc)
Attachment #352813 - Flags: superreview+
Attachment #352813 - Flags: review?(roc)
Attachment #352813 - Flags: review+
Attached patch patch (obsolete) — Splinter Review
The tests that Jesse encouraged me to add exposed that another change was needed to prevent the tests from crashing; see the additional change to nsPopupSetFrame.

Ran all reftests, chrome tests, and browser-chrome tests.

Unfortunately, the act of adding the additional tests causes 446100-1[efgh].html to start breaking in my tree; they're showing yellow boxes.  (If I comment out 421203-6, then only f and g break.)

I'm a little suspicious of some weird side-effect of the canvas caching...
Attachment #352813 - Attachment is obsolete: true
Attachment #352852 - Flags: superreview?
Attachment #352852 - Flags: review?
Attachment #352852 - Flags: superreview?(roc)
Attachment #352852 - Flags: superreview?
Attachment #352852 - Flags: review?(roc)
Attachment #352852 - Flags: review?
Comment on attachment 352852 [details] [diff] [review]
patch

I think this patch is OK; I'll continue to debug the reftest issue.  (Disabling the canvas caching does fix it.  Next thing to test will be to see if copying the references so we don't increase the canvas cache count across those tests also fixes it.)
zwol also hit problems with those reftests in bug 454349 comment 25.
Comment on attachment 352852 [details] [diff] [review]
patch

I'd prefer to see that "find the root box" code in nsLayoutUtils or possibly nsPresShell or nsFrameManager.
Attachment #352927 - Flags: superreview?(roc)
Attachment #352927 - Flags: superreview+
Attachment #352927 - Flags: review?(roc)
Attachment #352927 - Flags: review+
Are there other places that do "find the root box"?   That code is also "find the root box from a popupset"...
Yes there are... turns out they call nsIRootBox::GetRootBox, so let's call that here too :-).
Attached patch patch (obsolete) — Splinter Review
Calls nsIRootBox::GetRootBox.
Attachment #352852 - Attachment is obsolete: true
Attachment #353087 - Flags: superreview?(roc)
Attachment #353087 - Flags: review?(roc)
Attachment #352852 - Flags: superreview?(roc)
Attachment #352852 - Flags: review?(roc)
I think we need to make that conditional on rootBox not being null. IIRC you can restyle a XUL <window> root element to display:table and end up with a document that has a popupset but no root box.
As requested, and also changes nsPopupSetFrame::Init to match.
Attachment #353087 - Attachment is obsolete: true
Attachment #353299 - Flags: superreview?(roc)
Attachment #353299 - Flags: review?(roc)
Attachment #353087 - Flags: superreview?(roc)
Attachment #353087 - Flags: review?(roc)
Attachment #353299 - Flags: superreview?(roc)
Attachment #353299 - Flags: superreview+
Attachment #353299 - Flags: review?(roc)
Attachment #353299 - Flags: review+
Landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/3555a8de08b2
http://hg.mozilla.org/mozilla-central/rev/4a73efb88edc
... along with bug 454349.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [landed on mozilla-central; baking]
Target Milestone: --- → mozilla1.9.2a1
I don't think we can land this on 1.9.1 given what we've said about extension compatibility.
(In reply to comment #26)
> I don't think we can land this on 1.9.1 given what we've said about extension
> compatibility.

Yeah, looks like it breaks Speed Dial 0.7.2.7, for one.

Also, some other possibly related discussion here:
http://forums.mozillazine.org/viewtopic.php?p=5258105&sid=742a848b5fc8306b55332a716358951d#p5258105
(In reply to comment #26)
> I don't think we can land this on 1.9.1 given what we've said about extension
> compatibility.
What exactly would cause extensions to break and what would extensions authors need to do to fix things?

Attached are screenshots of Chatzilla and Auto Dial; and both have some degree of oddness. Chatzilla is totally unusable with layout all messed up. Auto Dial isn't too bad, but each row has equalsize=always with items flex=1, but it seems that if the content doesn't need to be cropped, flex doesn't kick in (difference in gray background color).

And just a sanity check, reverting to 4a73efb88edc shows the problem but 5bf3661db065 is fine.
Extension authors would need to look at the error console (which will have a message describing why the root element's contents were wrapped in a block) and make the offending item described there 'display:none', remove it, or comment it out, depending on what was desired.

Chatzilla trunk is already fixed; see bug 469983.
Whiteboard: [landed on mozilla-central; baking] → [landed on mozilla-central; probably can't land for 1.9.1]
Flags: in-testsuite+
Here's a branch wallpaper patch.  The test is the same as trunk.

I propose landing this on the 1.9.1 branch only.
Attachment #360787 - Flags: superreview?(roc)
Attachment #360787 - Flags: review?(roc)
I should probably explicitly set mBlockFrame = nsnull as well.
Attachment #360787 - Flags: superreview?(roc)
Attachment #360787 - Flags: superreview+
Attachment #360787 - Flags: review?(roc)
Attachment #360787 - Flags: review+
Landed wallpaper on 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c37466180679
Keywords: fixed1.9.1
Whiteboard: [landed on mozilla-central; probably can't land for 1.9.1]
David, can you please enlighten me for verifying this bug? When I use the self-contained testcase (attachment 338407 [details]) Minefield is presenting a blank page while Shiretoko displays our tree image repeated in x and y direction. I don't think that this is expected, or?
It seems to me that Minefield is showing the correct behavior. When an elements is display: inline, it doesn't seem to me that flex should make any difference anymore. Also, the width of the element becomes 0 on trunk, which is what I also would expect for an empty inline element.

Verified fixed (for not crashing anymore, at least):
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090208 Shiretoko/3.1b3pre (.NET CLR 3.5.30729)
Status: RESOLVED → VERIFIED
I noticed this also crashes on 1.9.0.x, is the branch wallpaper fix also suitable for 1.9.0.x?
Flags: blocking1.9.0.8?
Flags: blocking1.9.0.7?
Flags: blocking1.9.0.7?
Flags: wanted1.9.0.x+
(In reply to comment #34)
> It seems to me that Minefield is showing the correct behavior. When an elements
> is display: inline, it doesn't seem to me that flex should make any difference
> anymore. Also, the width of the element becomes 0 on trunk, which is what I
> also would expect for an empty inline element.

So that means Shiretoko doesn't handle it correctly? Shall we file a new bug on that? Martijn, could you do that? You have more insight what should going on here.
We should take this in 1.9.0 too
Flags: blocking1.9.0.8? → blocking1.9.0.8+
Comment on attachment 360787 [details] [diff] [review]
branch wallpaper
[Checkin: Comment 32 & 39]

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #360787 - Flags: approval1.9.0.8? → approval1.9.0.8+
Checked in to CVS trunk for 1.9.0.8, 2009-03-08 12:15 -0700.
Keywords: fixed1.9.0.8
Verified for 1.9.0.8 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009031904 GranParadiso/3.0.8pre. 

Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090319 Shiretoko/3.5b4pre.
Attachment #352927 - Attachment description: fix reftest-zoom tests to paint entire canvas → fix reftest-zoom tests to paint entire canvas [Checkin: Comment 24]
Attachment #353299 - Attachment description: patch → patch [Checkin: Comment 24]
Attachment #360787 - Attachment description: branch wallpaper → branch wallpaper [Checkin: Comment 32 & 39]
Blocks: 321056
Crash Signature: [@ InlineBackgroundData::GetContinuousRect]
See Also: → 1209896
You need to log in before you can comment on or make changes to this bug.