437142 - [FIX]Crash [@ nsStyleContext::Destroy] with image map, MathML

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008053113 Minefield/3.1a1pre

Steps to reproduce:
1. Load the testcase.
2. Reload or close the window (but don't navigate to another page first!).

Result: crash [@ nsStyleContext::Destroy] calling 0xdddddddd.

Comment 1

[Jesse Ruderman]

Attachment: crash report

Comment 2

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

Seems like a style context in the undisplayed map has a pointer to a rule node that's already been deleted.

Comment 3

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

So the undisplayed node on which we're crashing had previously been through nsFrameManager::ReResolveStyleContext, through the codepath in which we note that it is no longer display:none so we enqueue a framechange; this means we leave the old style context in the undisplayed map on the assumption that it will shortly be destroyed.

I don't yet understand why the undisplayed node wasn't destroyed shortly thereafter.  But presumably the caller to ReResolveStyleContext destroyed the old rule tree in its entirety, causing the dangling pointer.

I'd also note that we're going through ReResolveStyleContext twice for the same node in pretty rapid succession (and we re-resolve to two different new style contexts).

Comment 4

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

And shortly before hitting that ReResolveStyleContext, I hit the assertion:

###!!! ASSERTION: frame/content mismatch: '!aPrimaryFrame || aPrimaryFrame->GetContent() == aContent', file /home/dbaron/builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10067

Comment 5

[Boris Zbarsky [:bzbarsky]]

This comes down to the <area> primary frame mess.  We only remove undisplayed content entries if the content node doesn't have a primary frame... but <area>s claim to.  See this block in nsCSSFrameConstructor::ContentRemoved:

  nsIFrame* childFrame =
    mPresShell->FrameManager()->GetPrimaryFrameFor(aChild, aIndexInContainer);
  if (! childFrame) {
    frameManager->ClearUndisplayedContentIn(aChild, aContainer);
  }

We really need to fix bug 135040.

Comment 6

[Daniel Holbert [:dholbert]]

The testcase crashes my up-to-date mozilla-central debug build, on Linux.  OS/Hardware --> All.

Comment 7

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

Boris, can you take this? If I can only assign one layout blocker to you, this seems like a good one :-).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This doesn't fix bug 135040, but should be nice and branch-safe.  I think the only place where GetPrimaryFrameFor(content) != content is for <area>s, and all this patch does is make the code that does frame removal and change processing aware of this <area> hackiness and work around it.  The ContentRemoved changes are enough to fix this bug, but the others are worth it for now, I think (if nothing else, to avoid Jesse's assert).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and I can probably take a few other layout blockers... good break from docshell.  :(

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: With the missing null-check added

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: mq patch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozlla.org/mozilla-central/rev/328c67561b8b but no test yet, of course.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 350047 [details] [diff] [review]
mq patch

This applies to 1.9 branch as-is, with some offsets.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Make that http://hg.mozilla.org/mozilla-central/rev/328c67561b8b

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 350047 [details] [diff] [review]
mq patch

Approved for 1.9.0.6, a=dveditz for release-drivers

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.9 branch.

Comment 17

[Alexander Sack]

is this something we would want on 1.8 branches too?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Is suspect so, yes.  Might take some checking to make sure there are no other changes needed there.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009010504 GranParadiso/3.0.6pre.

Comment 20

[Alexander Sack]

(In reply to comment #18)
> Is suspect so, yes.  Might take some checking to make sure there are no other
> changes needed there.

what to do with the accessibility stuff. should i just disable the code for when we get the prim frame content mismatch (how i would call it now for now)? like:

+  PRBool primaryFrameContentMismatch = PR_FALSE;
+  if (aPrimaryFrame && aPrimaryFrame->GetContent() != aContent) {
+    // XXXbz this is due to image maps messing with the primary frame mapping.
+    // See bug 135040.  We can remove this block once that's fixed.
+    aPrimaryFrame = nsnull;
+    primaryFrameContentMismatch = PR_TRUE;
+  }
 #ifdef ACCESSIBILITY
   nsIAtom *prevRenderedFrameType = nsnull;
-  if (mPresShell->IsAccessibilityActive()) {
+  if (!primaryFrameContentMismatch && mPresShell->IsAccessibilityActive()) {
     prevRenderedFrameType = GetRenderedFrameType(aPrimaryFrame);
   }


and

 #ifdef ACCESSIBILITY
-  if (mPresShell->IsAccessibilityActive()) {
+  if (!primaryFrameContentMismatch && mPresShell->IsAccessibilityActive()) {
     nsIFrame *frame;
     mPresShell->GetPrimaryFrameFor(aContent, &frame);
     NotifyAccessibleChange(prevRenderedFrameType,
                            GetRenderedFrameType(frame),
                            aContent);
   }

.. or just continue with prevRenderedFrameType being nsnull?

Thanks!

Comment 21

[Boris Zbarsky [:bzbarsky]]

I wouldn't worry about those callsites at all, since they don't deal with the frame tree structure.  That is, the worst-case scenario there is incorrect accessibility events being fired.  I was just patching the GetPrimaryFrameFor callsites that could lead to crashes due to the frame tree managemenent code getting confused.

Comment 22

[Tony Chung [:tchung]]

Crash is gone on 1.9.1branch and trunk, but error console throws: 

Error: The "coords" attribute of the <area shape="rect"> tag is not in the "left,top,right,bottom" format.
Source File: https://bugzilla.mozilla.org/attachment.cgi?id=323634
Line: 0
Source Code:
coords=""

Is this a problem, before i mark it verified?

Comment 23

[Martijn Wargers (dead)]

No, that's not a problem.

Comment 24

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.1b3pre) Gecko/20090123 Shiretoko/3.1b3pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090123 Minefield/3.2a1pre

Comment 25

[Alexander Sack]

Attachment: 1st 1.8.0

is there more to do for 1.8 backport?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 358823 [details] [diff] [review]
1st 1.8.0

Yeah, looks fine.

Comment 27

[Alexander Sack]

Comment on attachment 358823 [details] [diff] [review]
1st 1.8.0

please approve for 1.8.1

Comment 28

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

Comment on attachment 358823 [details] [diff] [review]
1st 1.8.0

> 
>+   
>+  // This is a nasty hack.  It needs to go away: see bug 135040.  Once this is
>+  // removed, the code added to nsCSSFrameConstructor::RestyleElement,
>+  // nsCSSFrameConstructor::ContentRemoved (both hacks there), and
>+  // nsCSSFrameConstructor::ProcessRestyledFrames to work around this issue can
>+  // be removed.

Any reason for adding the extra whitespace line? (I know, I know, I'm a nitpicker.) ;)

Approved for 1.8.1.21. a=ss

Comment 29

[Alexander Sack]

committed for 1.8.1.21 (without whitespace line):

Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1110.6.96; previous revision: 1.1110.6.95
done
Checking in layout/generic/nsImageMap.cpp;
/cvsroot/mozilla/layout/generic/nsImageMap.cpp,v  <--  nsImageMap.cpp
new revision: 3.111.4.6; previous revision: 3.111.4.5