Closed Bug 437142 Opened 16 years ago Closed 16 years ago

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


(Core :: Layout, defect, P2)






(Reporter: jruderman, Assigned: bzbarsky)


(Depends on 1 open bug)


(5 keywords, Whiteboard: [sg:critical])

Crash Data


(4 files, 2 obsolete files)

Attached file 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.
Attached file crash report
Whiteboard: [sg:critical]
Seems like a style context in the undisplayed map has a pointer to a rule node that's already been deleted.
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).
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
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.
Depends on: 135040
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
The testcase crashes my up-to-date mozilla-central debug build, on Linux.  OS/Hardware --> All.
OS: Mac OS X → All
Hardware: PC → All
Boris, can you take this? If I can only assign one layout blocker to you, this seems like a good one :-).
Assignee: nobody → bzbarsky
Attached patch Proposed fix (obsolete) — Splinter Review
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).
Attachment #347861 - Flags: superreview?(roc)
Attachment #347861 - Flags: review?(roc)
Summary: Crash [@ nsStyleContext::Destroy] with image map, MathML → [FIX]Crash [@ nsStyleContext::Destroy] with image map, MathML
Oh, and I can probably take a few other layout blockers... good break from docshell.  :(
Attachment #347861 - Flags: superreview?(roc)
Attachment #347861 - Flags: superreview+
Attachment #347861 - Flags: review?(roc)
Attachment #347861 - Flags: review+
Whiteboard: [sg:critical] → [sg:critical][needs landing]
Attached patch mq patchSplinter Review
Attachment #347861 - Attachment is obsolete: true
Attachment #348211 - Attachment is obsolete: true
Pushed but no test yet, of course.
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment on attachment 350047 [details] [diff] [review]
mq patch

This applies to 1.9 branch as-is, with some offsets.
Attachment #350047 - Flags: approval1.9.0.6?
Target Milestone: --- → mozilla1.9.1b3
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.6+
Whiteboard: [sg:critical][needs landing] → [sg:critical]
Comment on attachment 350047 [details] [diff] [review]
mq patch

Approved for, a=dveditz for release-drivers
Attachment #350047 - Flags: approval1.9.0.6? → approval1.9.0.6+
Fixed on 1.9 branch.
Keywords: fixed1.9.0.6
Keywords: fixed1.9.1
Target Milestone: mozilla1.9.1b3 → mozilla1.9.1b2
is this something we would want on 1.8 branches too?
Flags: wanted1.8.1.x?
Flags: wanted1.8.0.x?
Is suspect so, yes.  Might take some checking to make sure there are no other changes needed there.
Verified fixed in with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: Gecko/2009010504 GranParadiso/3.0.6pre.
(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;
+  }
   nsIAtom *prevRenderedFrameType = nsnull;
-  if (mPresShell->IsAccessibilityActive()) {
+  if (!primaryFrameContentMismatch && mPresShell->IsAccessibilityActive()) {
     prevRenderedFrameType = GetRenderedFrameType(aPrimaryFrame);


-  if (mPresShell->IsAccessibilityActive()) {
+  if (!primaryFrameContentMismatch && mPresShell->IsAccessibilityActive()) {
     nsIFrame *frame;
     mPresShell->GetPrimaryFrameFor(aContent, &frame);

.. or just continue with prevRenderedFrameType being nsnull?

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.
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:
Line: 0
Source Code:

Is this a problem, before i mark it verified?
No, that's not a problem.
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
Attached patch 1st 1.8.0Splinter Review
is there more to do for 1.8 backport?
Attachment #358823 - Flags: review?(bzbarsky)
Comment on attachment 358823 [details] [diff] [review]
1st 1.8.0

Yeah, looks fine.
Attachment #358823 - Flags: review?(bzbarsky) → review+
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: wanted1.8.0.x+
Attachment #358823 - Flags:
Attachment #358823 - Flags:
Comment on attachment 358823 [details] [diff] [review]
1st 1.8.0

please approve for 1.8.1
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 a=ss
Attachment #358823 - Flags: →
committed for (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
Checking in layout/generic/nsImageMap.cpp;
/cvsroot/mozilla/layout/generic/nsImageMap.cpp,v  <--  nsImageMap.cpp
new revision:; previous revision:
Keywords: fixed1.8.1.21
Flags: →
Group: core-security
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsStyleContext::Destroy]
Depends on: 824641
You need to log in before you can comment on or make changes to this bug.