Last Comment Bug 437142 - [FIX]Crash [@ nsStyleContext::Destroy] with image map, MathML
: [FIX]Crash [@ nsStyleContext::Destroy] with image map, MathML
Status: VERIFIED FIXED
[sg:critical]
: crash, fixed1.8.1.21, testcase, verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P2 critical (vote)
: mozilla1.9.1b2
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on: 135040 824641
Blocks: randomstyles 347580
  Show dependency treegraph
 
Reported: 2008-06-03 16:47 PDT by Jesse Ruderman
Modified: 2012-12-25 14:14 PST (History)
14 users (show)
roc: blocking1.9.1+
dveditz: blocking1.9.0.6+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
asac: wanted1.8.1.x+
asac: blocking1.8.0.next+
asac: wanted1.8.0.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (606 bytes, text/html)
2008-06-03 16:47 PDT, Jesse Ruderman
no flags Details
crash report (40.91 KB, text/plain)
2008-06-03 16:52 PDT, Jesse Ruderman
no flags Details
Proposed fix (4.56 KB, patch)
2008-11-12 14:14 PST, Boris Zbarsky [:bz] (still a bit busy)
roc: review+
roc: superreview+
Details | Diff | Splinter Review
With the missing null-check added (4.57 KB, patch)
2008-11-14 10:06 PST, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
mq patch (4.73 KB, patch)
2008-11-25 13:38 PST, Boris Zbarsky [:bz] (still a bit busy)
dveditz: approval1.9.0.6+
Details | Diff | Splinter Review
1st 1.8.0 (4.64 KB, patch)
2009-01-26 04:15 PST, Alexander Sack
bzbarsky: review+
samuel.sidler+old: approval1.8.1.next+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Jesse Ruderman 2008-06-03 16:47:43 PDT
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.
Comment 1 Jesse Ruderman 2008-06-03 16:52:33 PDT
Created attachment 323637 [details]
crash report
Comment 2 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-06-04 10:15:04 PDT
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-7 (busy September 14-25) 2008-06-04 10:48:40 PDT
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-7 (busy September 14-25) 2008-06-04 10:50:12 PDT
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 [:bz] (still a bit busy) 2008-09-01 16:42:50 PDT
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] 2008-10-03 15:19:17 PDT
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) 2008-11-11 18:46:35 PST
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 [:bz] (still a bit busy) 2008-11-12 14:14:56 PST
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).
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2008-11-12 14:15:49 PST
Oh, and I can probably take a few other layout blockers... good break from docshell.  :(
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2008-11-14 10:06:28 PST
Created attachment 348211 [details] [diff] [review]
With the missing null-check added
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2008-11-25 13:38:42 PST
Created attachment 350047 [details] [diff] [review]
mq patch
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2008-11-25 17:55:42 PST
Pushed http://hg.mozlla.org/mozilla-central/rev/328c67561b8b but no test yet, of course.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2008-11-25 17:56:53 PST
Comment on attachment 350047 [details] [diff] [review]
mq patch

This applies to 1.9 branch as-is, with some offsets.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2008-11-25 18:01:05 PST
Make that http://hg.mozilla.org/mozilla-central/rev/328c67561b8b
Comment 15 Daniel Veditz [:dveditz] 2008-12-08 11:54:33 PST
Comment on attachment 350047 [details] [diff] [review]
mq patch

Approved for 1.9.0.6, a=dveditz for release-drivers
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2008-12-10 09:46:53 PST
Fixed on 1.9 branch.
Comment 17 Alexander Sack 2009-01-04 17:37:12 PST
is this something we would want on 1.8 branches too?
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2009-01-04 21:20:47 PST
Is suspect so, yes.  Might take some checking to make sure there are no other changes needed there.
Comment 19 Al Billings [:abillings] 2009-01-05 17:09:53 PST
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 2009-01-08 05:32:19 PST
(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 [:bz] (still a bit busy) 2009-01-08 09:00:54 PST
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] 2009-01-23 12:26:10 PST
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 [:mwargers] (not working for Mozilla) 2009-01-23 13:12:04 PST
No, that's not a problem.
Comment 24 Tony Chung [:tchung] 2009-01-23 13:13:23 PST
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 2009-01-26 04:15:03 PST
Created attachment 358823 [details] [diff] [review]
1st 1.8.0

is there more to do for 1.8 backport?
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2009-01-26 06:51:35 PST
Comment on attachment 358823 [details] [diff] [review]
1st 1.8.0

Yeah, looks fine.
Comment 27 Alexander Sack 2009-01-26 07:14:05 PST
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) 2009-01-26 07:28:02 PST
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 2009-01-26 07:47:27 PST
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

Note You need to log in before you can comment on or make changes to this bug.