387544 - [FIXr]Crash [@ nsFrameManager::ReResolveStyleContext] with position: absolute, display: table-caption and setting display: none

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk and branch builds within 300ms, normally.
Because it also crashes branch builds, I'm marking it security sensitive for now.

The data url consists of this:
<html><head></head>
<body style="display: table;">
<div></div>
<div id="a" style="display: table-caption;">
  <div id="b" style="position: absolute;"></div>
</div>
</body>
</html>

http://crash-stats.mozilla.com/report/index/397dd1b3-2ee5-11dc-bc9f-001a4bd43ed6
0  	nsFrameManager::ReResolveStyleContext(nsPresContext *,nsIFrame *,nsIContent *,nsStyleChangeList *,nsChangeHint)
1 	nsFrameManager::ComputeStyleChangeFor(nsIFrame *,nsStyleChangeList *,nsChangeHint)
2 	nsCSSFrameConstructor::RestyleElement(nsIContent *,nsIFrame *,nsChangeHint)
3 	nsCSSFrameConstructor::ProcessOneRestyle(nsIContent *,nsReStyleHint,nsChangeHint)
4 	nsCSSFrameConstructor::ProcessPendingRestyles()
5 	nsCSSFrameConstructor::RestyleEvent::Run()
6 	nsThread::ProcessNextEvent(int,int *)
7 	NS_ProcessNextEvent_P(nsIThread *,int)
8 	nsBaseAppShell::Run()
9 	nsAppStartup::Run()

Talkback ID: TB33900982G

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk fix

The patch is pretty self-explanatory: processing pseudoframes when constructing the caption means that tempItems ends up with both the pseudo rowgroup and the caption.  So we were failing to parent the caption correctly, and things broke.

I'm not sure how to fix this on branch, offhand, without porting the fix for bug 243159 first.  Unfortunately, we really do need to detect the caption post-facto, and with the branch setup that's too late....  I suppose we could pass around a caption frame pointer all over (as TableProcessChild does), but that would really suck...

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 274425 [details] [diff] [review]
Trunk fix

We should use nsFrameList:: GetPrevSiblingFor here, for the !aPrevSibling case, but I'll let it slide since that's just existing code.

I assume the caption frames can't be wrapped in anything that would hide them from this search?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I assume the caption frames can't be wrapped in anything that would hide them
> from this search?

Right.  Not if the parent is a table, as here.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 275027 [details] [diff] [review]
Merged to tip

This fixes a crash by making sure we don't make assumptions that just aren't true about how many frames will get added to a framelist when constructing frames for a single content node.

The risk is moderate shading into low; about as good as this code ever gets.

Comment 6

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

Comment on attachment 275027 [details] [diff] [review]
Merged to tip

a1.9=dbaron

Comment 7

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 8

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080805 Minefield/3.0a8pre

Comment 9

[Daniel Veditz [:dveditz]]

From bug 398404 comment 2:
  [bug 387544] is a _lot_ more work and needs pretty extensive testing
  for any patch we come up with.  See bug 387544 comment 1.  It shouldn't
  be going in this close to the end of a branch cycle.  :(

Unfortunately people tend not to pay attention to their branch bugs until the end of the cycle (the trunk fix here was done at the beginning of this cycle) :-(

Moved blocking flag to 1.8.1.9. Tomorrow starts the 1.8.1.9 dev cycle while QA/Build finishes up the 1.8.1.8 release

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm going to need some help here, I suspect, if we're to get this fixed on branch.

Comment 11

[Daniel Veditz [:dveditz]]

In email Boris says for the branch we have a choice between risky (port biggish frame construction changes to the branch) or tricky (some custom hack), and it's really too late in 1.8.1.12 for either.

Comment 12

[Samuel Sidler (old account; do not CC)]

Boris, who would be able to help you on getting this ported to branch? (i.e., who can we ask?)

Comment 13

[Boris Zbarsky [:bzbarsky]]

I think the only people who can even claim to know this code are bernd, roc, mats, and dbaron (probably in that order)...

Comment 14

[Boris Zbarsky [:bzbarsky]]

Just to make it clear, I don't think there's going to be a small safe patch here.  Either you'll have a big risky backport or a big, tedious, less tested, so possibly just as risky different patch altogether...

Comment 15

[Samuel Sidler (old account; do not CC)]

(In reply to comment #14)
> Just to make it clear, I don't think there's going to be a small safe patch
> here.  Either you'll have a big risky backport or a big, tedious, less tested,
> so possibly just as risky different patch altogether...

Sadly, that's very clear, but we still want to push for getting this fixed. If not this cycle, then the next.

Bernd, do you have time to look at porting this to the 1.8 branch (or creating a new patch entirely)?

Comment 16

[Martijn Wargers (dead)]

See bug 415447, comment 12.
Let's just stop wasting time trying to fix weird crashes on branch and just focus on branch on top-crashes or public security issues.

Comment 17

[Bernd]

>Bernd, do you have time to look at porting this to the 1.8 branch (or creating
>a new patch entirely)?
No, comment 14 says it very clearly, the required changes exceed my capabilities and I still have 1.9 wanteds on my list.

Comment 18

[Samuel Sidler (old account; do not CC)]

We don't have anyone that can own this right now. Minusing for 1.8.1.13.