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

VERIFIED FIXED in mozilla1.9.1b2

Status

()

Core
Layout
P2
critical
VERIFIED FIXED
9 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, 5 keywords)

Trunk
mozilla1.9.1b2
crash, fixed1.8.1.21, testcase, verified1.9.0.6, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.6 +
wanted1.9.0.x +
blocking1.8.1.next +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 323634 [details]
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.
(Reporter)

Comment 1

9 years ago
Created attachment 323637 [details]
crash report
(Reporter)

Updated

9 years ago
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
(Reporter)

Updated

9 years ago
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
Created attachment 347861 [details] [diff] [review]
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).
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+
Created attachment 348211 [details] [diff] [review]
With the missing null-check added
Whiteboard: [sg:critical] → [sg:critical][needs landing]
Created attachment 350047 [details] [diff] [review]
mq patch
Attachment #347861 - Attachment is obsolete: true
Attachment #348211 - Attachment is obsolete: true
Pushed http://hg.mozlla.org/mozilla-central/rev/328c67561b8b but no test yet, of course.
Status: NEW → RESOLVED
Last Resolved: 9 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?
Make that http://hg.mozilla.org/mozilla-central/rev/328c67561b8b
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 1.9.0.6, 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

Comment 17

9 years ago
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 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.
Keywords: fixed1.9.0.6 → verified1.9.0.6

Comment 20

9 years ago
(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!
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: https://bugzilla.mozilla.org/attachment.cgi?id=323634
Line: 0
Source Code:
coords=""

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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1

Comment 25

9 years ago
Created attachment 358823 [details] [diff] [review]
1st 1.8.0

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+

Updated

9 years ago
Flags: wanted1.8.1.x?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x?
Flags: wanted1.8.0.x+

Updated

9 years ago
Flags: blocking1.8.1.next?
Flags: blocking1.8.0.next+

Updated

9 years ago
Attachment #358823 - Flags: approval1.8.1.next?
Attachment #358823 - Flags: approval1.8.0.next+

Comment 27

9 years ago
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 1.8.1.21. a=ss
Attachment #358823 - Flags: approval1.8.1.next? → approval1.8.1.next+

Comment 29

9 years ago
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
Keywords: fixed1.8.1.21
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Group: core-security
(Reporter)

Updated

9 years ago
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.