Last Comment Bug 391178 - Crash [@ nsCSSFrameConstructor::FindFrameWithContent] with XUL trees, position:fixed
: Crash [@ nsCSSFrameConstructor::FindFrameWithContent] with XUL trees, positio...
Status: VERIFIED FIXED
[sg:critical?][dbaron-1.9:Rs]
: crash, testcase, verified1.8.1.15
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 All
: P2 critical (vote)
: ---
Assigned To: Daniel Holbert [:dholbert]
:
:
Mentors:
Depends on:
Blocks: randomstyles 344486 366583 397954 399715
  Show dependency treegraph
 
Reported: 2007-08-06 21:03 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
17 users (show)
dbaron: blocking1.9+
dveditz: blocking1.8.1.15+
jruderman: wanted1.8.1.x+
dholbert: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (700 bytes, application/xhtml+xml)
2007-08-06 21:04 PDT, Jesse Ruderman
no flags Details
possible fix (2.00 KB, patch)
2007-08-09 18:53 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
possible fix (1.31 KB, patch)
2007-08-09 18:56 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
testcase2 (637 bytes, application/vnd.mozilla.xul+xml)
2007-10-24 17:05 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
fix (2.73 KB, patch)
2007-11-05 12:19 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
better fix (using GetCachedTreeBody) (2.91 KB, patch)
2007-11-19 16:00 PST, Daniel Holbert [:dholbert]
roc: review+
roc: superreview+
Details | Diff | Splinter Review
branch patch v1 (3.23 KB, patch)
2008-03-26 19:40 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
backtrace of 'testcase' crash (using Branch / OS X) (2.53 KB, text/plain)
2008-05-05 14:04 PDT, Daniel Holbert [:dholbert]
no flags Details
branch patch v2 (10.03 KB, patch)
2008-05-05 15:38 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
backtrace of 'testcase' crash (using Branch / OS X) (detail) (5.70 KB, text/plain)
2008-05-05 17:00 PDT, Daniel Holbert [:dholbert]
no flags Details
branch patch v3 (9.98 KB, patch)
2008-05-05 17:11 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
branch patch v3b (10.15 KB, patch)
2008-05-05 17:23 PDT, Daniel Holbert [:dholbert]
roc: review+
roc: superreview+
dveditz: approval1.8.1.15+
Details | Diff | Splinter Review

Description Jesse Ruderman 2007-08-06 21:03:38 PDT
Thread 0 Crashed reading memory at 0xdddddddd:

0   nsCSSFrameConstructor::FindFrameWithContent(nsFrameManager*, nsIFrame*, nsIContent*, nsIContent*, nsFindFrameHint*) + 372 (nsCSSFrameConstructor.cpp:10773)
1   nsCSSFrameConstructor::FindPrimaryFrameFor(nsFrameManager*, nsIContent*, nsIFrame**, nsFindFrameHint*) + 237 (nsCSSFrameConstructor.cpp:10865)
2   nsFrameManager::GetPrimaryFrameFor(nsIContent*, int) + 538 (nsFrameManager.cpp:394)
3   PresShell::GetPrimaryFrameFor(nsIContent*) const + 42 (nsPresShell.cpp:4688)
4   nsBoxObject::GetFrame(int) + 102 (nsBoxObject.cpp:141)
5   nsTreeBoxObject::GetTreeBody() + 53 (nsTreeBoxObject.cpp:116)
6   nsTreeBoxObject::GetColumns(nsITreeColumns**) + 17 (nsTreeBoxObject.cpp:234)
7   nsTreeColFrame::InvalidateColumns() + 96 (nsTreeColFrame.cpp:228)
8   nsTreeColFrame::Destroy() + 17 (nsTreeColFrame.cpp:103)
9   nsFrameList::DestroyFrames() + 48 (nsFrameList.cpp:68)
10  nsContainerFrame::Destroy() + 73 (nsContainerFrame.cpp:255)
...
Comment 1 Jesse Ruderman 2007-08-06 21:04:22 PDT
Created attachment 275548 [details]
testcase
Comment 2 chris hofmann 2007-08-08 16:52:07 PDT
jan is probably not a good assignee for this bug.  I wonder who else could take a crack?
Comment 3 Daniel Holbert [:dholbert] 2007-08-08 17:26:19 PDT
Stealing bug from damon.
Comment 4 Daniel Holbert [:dholbert] 2007-08-09 16:46:28 PDT
Here's what's going wrong.  (Sorry that this post is a bit long.)

When we remove "display: fixed" from the div, that makes us destroy the div's  frame subtree.  During this process, we hit 
  nsTreeColFrame::Destroy
   nsTreeColFrame::InvalidateColumns
    nsITreeBoxObject::GetColumns
     nsTreeBoxObject::GetTreeBody
      nsITreeBoxObject::GetFrame
       PresShell::GetPrimaryFrameFor
        nsFrameManager::GetPrimaryFrameFor
         nsCSSFrameConstructor::FindPrimaryFrameFor

This lowest function call is trying to find the primary frame for "aContent", which is the nsTreeColFrame's TreeBoxObject.

(I'm not sure exactly what a TreeBoxObject is, but all that matters here is that it's a direct child of the div that's being destroyed.

Within that lowest function call (FindPrimaryFrameFor), we hit these lines:
 10878   nsCOMPtr<nsIContent> parentContent = aContent->GetParent();
 10879   if (parentContent) {
 10880     parentFrame = aFrameManager->GetPrimaryFrameFor(parentContent, -1);

Here, parentContent is the div that's being removed, and line 10880 tries to look up its "primary frame".

So, here's the kicker -- the div's true primary frame is the nsAreaFrame that we're halfway through destroying.  This frame has already been removed from the frame tree, so I think we don't want to be able to find it here -- we'd _like_ the variable "parentFrame" to end up being null. (** see note at end of this post)

HOWEVER, we actually _do_ find the div's area frame, even though it's been stitched out of the tree.  We find it via its placeholder, which _isn't_ yet stitched out. (here in the code: 
http://mxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#10769
)

As a result, we keep going through this call to FindPrimaryFrameFor, using a half-destroyed "parentFrame", and we pass that parent frame to "FindFrameWithContent" at line 10883.  This makes us walk parentFrame's subtree, and we segfault on a destroyed frame's invalid pointer. (with mContent ==  0xdddddddd)

*** Note from above: In a reversed test case where the div *starts* with no style and is later set to "position: fixed", the parentFrame variable _does_ end up being null at this point in the code, and there's no crash.  That's why I'm hypothesizing that we might like parentFrame to be null in the original test case, too...
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-08-09 17:01:41 PDT
One solution is probably to always add tree body frames to the primary frame map.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2007-08-09 17:35:48 PDT
That sounds like a good idea.
Comment 7 Daniel Holbert [:dholbert] 2007-08-09 18:42:52 PDT
(In reply to comment #5)
> One solution is probably to always add tree body frames to the primary frame
> map.

Thanks for the suggestion!  I looked into this, and as it turns out, the tree body frame actually is already put in the primary frame map, but it gets removed from the map before the crashy part of the code.

Specifically, in nsCSSFrameConstructor::ContentRemoved,

 - line 9565: we call ::DeletingFrameSubtree, which walks the to-be-destroyed subtree and removes all its mappings from the primary frame map

 - line 9583: we call frameManager->RemoveFrame, which encompasses all of the Destroy() calls and has the crash.

Any other ideas?
Comment 8 Daniel Holbert [:dholbert] 2007-08-09 18:53:32 PDT
Created attachment 276058 [details] [diff] [review]
possible fix

Here's one simple fix: just clear placeholderFrame.mOutOfFlowFrame before destroying the fixed area frame's subtree.

This fixes the test case.  Haven't tested it on much else yet (but will do so soon).

This solution might be dangerous -- roc said in IRC that this area of code is known to be fragile and has had issues before.
Comment 9 Daniel Holbert [:dholbert] 2007-08-09 18:55:25 PDT
(oops -- ignore the first chunk of that patch -- that part has no effect, just adds an unused variable.  The second chunk is what matters.)
Comment 10 Daniel Holbert [:dholbert] 2007-08-09 18:56:54 PDT
Created attachment 276059 [details] [diff] [review]
possible fix
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-08-09 19:11:21 PDT
I think we should avoid calling GetPlaceholderFrameFor at all during frame destruction.

In this case, we could have nsTreeColFrame hold a strong reference to the nsITreeColumns, and call InvalidateColumns on it directly.
Comment 12 neil@parkwaycc.co.uk 2007-08-10 01:22:39 PDT
(In reply to comment #11)
>In this case, we could have nsTreeColFrame hold a strong reference to the
>nsITreeColumns, and call InvalidateColumns on it directly.
That might be tricky under the current ownership model because it's still the nsTreeBodyFrame that creates the nsITreeColumns so that you might find you're moving the frame hunt to a different critical path, but it should be possible to make the nsTreeBoxObject own the columns instead. At least on trunk I think all the critical code already checks for mView which is also owned by the box.
Comment 13 neil@parkwaycc.co.uk 2007-08-10 01:26:44 PDT
Hmm, that's not so easy, because the nsTreeBodyFrame wants an nsTreeColumns...
Comment 14 Daniel Holbert [:dholbert] 2007-08-10 11:10:48 PDT
(In reply to comment #8)
> Created an attachment (id=276058) [details]
> possible fix
> 
> Here's one simple fix: just clear placeholderFrame.mOutOfFlowFrame before
> destroying the fixed area frame's subtree.
> 
> This fixes the test case.  Haven't tested it on much else yet (but will do so
> soon).
> 
> This solution might be dangerous -- roc said in IRC that this area of code is
> known to be fragile and has had issues before.
> 

That last patch passed the layout reftests... Do you guys think it's too risky?  It's certainly a simple solution, and it makes conceptual sense to me, based on the rest of the way we do frame destruction. 
(That is to say, our general strategy seems to be to break all ties from the frame tree to the pruned branch before we do any frame destruction.  And currently, placeholder frames violate this strategy.)

Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-08-10 16:31:19 PDT
I guess I'm OK with it if Boris and David are. Maybe you could find the bugs that touched the placeholder cleanup code and recheck their testcases?
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2007-08-10 16:45:36 PDT
Does blockframe not rely on the float placeholders when tearing down the float?  See nsBlockFrame::RemoveFloat.  I thought that was the reason the ordering here was as it is?
Comment 17 Daniel Holbert [:dholbert] 2007-08-13 17:49:27 PDT
D'oh, looks like bz's right -- we get this chain of calls:

  nsBlockFrame::RemoveFloat(nsIFrame* aFloat)
   nsLineBox::RemoveFloat(nsIFrame* aFrame)
    nsFloatCacheList::Find(nsIFrame* aOutOfFlowFrame)
   
which iterates through the line's placeholders and says

     (fc->mPlaceholder->GetOutOfFlowFrame() == aOutOfFlowFrame)

And actually, it looks like the XXX comment near nsCSSFrameConstructor.cpp:9581 basically says that :)  My bad -- I'd misread it before, and had thought we were trying to preserve the pointer in the *other* direction.
Comment 18 Daniel Holbert [:dholbert] 2007-10-24 15:16:18 PDT
Interesting... this seems to have become WORKSFORME.  Does anyone else still see the crash?

Gonna see if I can figure out what fixed it.
Comment 19 Daniel Holbert [:dholbert] 2007-10-24 15:52:48 PDT
In a current trunk build, I'm getting this assertion failure instead of the crash:

###!!! ASSERTION: Null out-of-flow for placeholder?: 'outOfFlow', file /scratch/work/builds/trunk.07-10-24.10-57/mozilla/layout/base/../generic/nsPlaceholderFrame.h, line 175
Comment 20 Daniel Holbert [:dholbert] 2007-10-24 15:55:10 PDT
(In reply to comment #19)
> In a current trunk build, I'm getting this assertion failure instead of the
> crash:
> 
> ###!!! ASSERTION: Null out-of-flow for placeholder?

... which is already filed as bug 399715, which this bug blocks.
Comment 21 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-10-24 17:05:55 PDT
Created attachment 286091 [details]
testcase2

I'm still crashing with this testcase after 100ms using current trunk build.
Comment 22 Daniel Holbert [:dholbert] 2007-11-05 12:19:54 PST
Created attachment 287443 [details] [diff] [review]
fix

This patch fixes the crash in testcase2.  The problem is that nsTreeBoxObject::GetTreeBody is walking the frame tree during frame destruction, which is bad.

The idea of this patch is to clear the nsTreeBoxObject's mContent pointer during destruction, to thwart its ability to traverse the frame tree in GetTreeBody.

Here's what happens, after this patch:
 - The added call to Clear clears the TreeBoxObject's mContent pointer. (via nsBoxObject::Clear)
 - In GetTreeBody(), if we don't already have mTreeBody cached (and we don't in this testcase), we now won't be able to look it up, because nsBoxObject::GetFrame() is going to return null.
    ** GetFrame will return null because mContent is null, and it uses mContent to look up the frame.
  - This makes GetTreeBody() just return null without doing any damage.

One slightly messy bit -- I do have to cast the nsITreeBoxObject to a nsTreeBoxObject in order to access Clear(), because it's not part of the nsITreeBoxObject interface.  There's probably a cleaner way to do this.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-11-05 13:16:10 PST
+      ((nsTreeBoxObject*)treeBoxObject)->Clear(); // clears its mContent pointer

This is OK, just use a static_cast. These interfaces are bogus anyway.

My question is, why does nsTreeColFrame::Destroy need to call InvalidateColumns at all? With your patch, it's not really going to do anything. So are we going to leak or something?
Comment 24 Daniel Holbert [:dholbert] 2007-11-05 15:55:18 PST
Hrm, good question.  I'd originally been thinking that the treeBoxObject's mTreeBody would still be around in cases where it was non-null, but I guess Clear() clears that as well.  So you're right, InvalidateColumns wouldn't do anything when called by Destroy, after my patch.


From exploring the source, it looks like all that nsTreeColFrame::InvalidateColumns() does is:
  - Get the TreeBoxObject
  - In GetColumns:
    * Try to look up the TreeBody
    * Try to get a nsCOM reference to the TreeBody's mColumns
  - If the above succeeded, we call InvalidateColumns on the returned nsITreeColumns, which just does this:

 552 NS_IMETHODIMP
 553 nsTreeColumns::InvalidateColumns()
 554 {
 555   NS_IF_RELEASE(mFirstColumn);
 556   return NS_OK;
 557 }

Note that nsTreeColumns::~nsTreeColumns is similar:
 373 nsTreeColumns::~nsTreeColumns()
 374 {
 ...   ...
 378   NS_IF_RELEASE(mFirstColumn);
 379 }


I'm still not totally familiar with how NS_IF_RELEASE / NS_IF_ADDREF are used and how the whole tree-object structure fits together, but it looks to me like we'll effectively do the work of InvalidateColumns() when (if?) the nsTreeColumns object is destroyed.

So, the InvalidateColumns in nsTreeColFrame::Destroy() may be redundant.
Comment 25 neil@parkwaycc.co.uk 2007-11-05 16:03:19 PST
My understanding is that under normal circumstances, i.e. when you're removing a column from a tree, rather than whatever Jesse managed to do to the frame tree, you need to invalidate the columns so that the next time the tree paints it won't try to paint deleted columns.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-11-07 18:24:28 PST
Well then, shouldn't whatever removes the column invalidate the columns, instead of relying on frame destruction to do it? A frame might not have been constructed for some reason.
Comment 27 neil@parkwaycc.co.uk 2007-11-08 05:23:45 PST
(In reply to comment #26)
>A frame might not have been constructed for some reason.
Then no invalidation should be necessary. (Not that this justifies the code.)
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-11-08 13:05:33 PST
I see.

Seems to me then that the simplest way to fix this is to have nsTreeColFrame carry a strong ref to nsITreeColumns and call InvalidateColumns directly on that.

In the bigger picture, how bad would it be for performance if we stopped caching the columnn list and just build or iterate it every time we need it?
Comment 29 neil@parkwaycc.co.uk 2007-11-08 13:52:00 PST
(In reply to comment #28)
>Seems to me then that the simplest way to fix this is to have nsTreeColFrame
>carry a strong ref to nsITreeColumns and call InvalidateColumns directly on that.
I think the difficulty here might be that multiple nsTreeColumns object can be created during the lifetime of an nsITreeColFrame, since they are currently created by the nsTreeBodyFrame object which is the <treechildren>'s frame.
On the other hand I don't think that nsTreeColFrame can outlive nsTreeBoxObject so it may be possible to rewrite nsTreeBoxObject to cache the nsTreeColumns.
Comment 30 Daniel Holbert [:dholbert] 2007-11-19 16:00:54 PST
Created attachment 289406 [details] [diff] [review]
better fix (using GetCachedTreeBody)

This fix is similar to the previous one, in that it aims to make  nsTreeColFrame::InvalidateColumns' frame-tree-walking behavior depend on whether we're destructing frames.

This new patch makes use of the "GetCachedTreeBody" function added in Bug 397954, to cleanly avoid the tree-walking behavior within nsTreeBoxObject::GetTreeBody.
Comment 31 Daniel Holbert [:dholbert] 2007-11-19 16:08:31 PST
'better fix' fixes: 
 - testcase2 on this bug (no crash or assertions)
 - testcase on Bug 366583 (no more ASSERTION: Null out-of-flow for placeholder?)
 - testcase on Bug 399715 (no more ASSERTION: Null out-of-flow for placeholder?)
Comment 32 Daniel Holbert [:dholbert] 2007-11-19 18:02:06 PST
'better fix' checked in:

Checking in nsTreeColFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeColFrame.cpp,v  <--  nsTreeColFrame.cpp
new revision: 1.52; previous revision: 1.51
done
Checking in nsTreeColFrame.h;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeColFrame.h,v  <--  nsTreeColFrame.h
new revision: 1.31; previous revision: 1.30
done
Comment 33 Carsten Book [:Tomcat] 2008-02-09 15:07:06 PST
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020819 Minefield/3.0b4pre and the testcases from this bug. 

No crash on testcases -> Verified fixed
Comment 34 Jesse Ruderman 2008-03-26 17:14:50 PDT
This bug affects branch.  Branch crashes in the same way that trunk used to -- [@ nsCSSFrameConstructor::FindFrameWithContent] dereferencing 0xdddddddd.
Comment 35 Daniel Holbert [:dholbert] 2008-03-26 19:04:46 PDT
I get no crashes on testcase / testcase2, using this build:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/20080325 Ubuntu/8.04 (hardy) Firefox/2.0.0.13

Jesse, what version of branch did you see this crash on?
Comment 36 Jesse Ruderman 2008-03-26 19:16:34 PDT
I tested with a debug build from last night.
Comment 37 Daniel Holbert [:dholbert] 2008-03-26 19:40:54 PDT
Created attachment 311950 [details] [diff] [review]
branch patch v1

Trunk patch seems to apply cleanly on branch, with a little fuzz.  This patch is the result of doing just that, and then diffing to get a no-fuzz-needed version.

Jesse, could you apply this to your debug build and see if this 
 a) compiles
 b) fixes the branch crash?
Comment 38 Daniel Holbert [:dholbert] 2008-03-27 10:59:05 PDT
Comment on attachment 311950 [details] [diff] [review]
branch patch v1

Nevermind, this branc patch needs a bit more work -- it doesn't compile in its current state, because there's no nsTreeBoxObject.h header and no "GetCachedTreeBody" function on branch.
Comment 39 Daniel Holbert [:dholbert] 2008-05-05 13:19:30 PDT
Per blocking1.8.1.15 status, I'll get a branch patch for this ASAP.

My current behavior on Mac OS X, using a branch checkout from today, is:

 -'testcase': (attachment 275548 [details])
   * hang, and then segfaults within 30 sec.
   * No assertion failures
   * These warnings:
WARNING: Write failed (non-fatal), file nsInputStreamTee.cpp, line 84
WARNING: NS_ENSURE_TRUE(aContent) failed, file nsFrameManager.cpp, line 339

 - 'testcase2' (attachment 286091 [details])
   * No hang/crash
   * These warnings & assertions are displayed:
WARNING: Write failed (non-fatal), file nsInputStreamTee.cpp, line 84
WARNING: waaah!, file nsXULPrototypeDocument.cpp, line 862
###!!! ASSERTION: wrong kind of child frame: 'aIsBlock == f->GetStyleDisplay()->IsBlockLevel()', file nsLineBox.cpp, line 70
###!!! ASSERTION: frame was not removed from primary frame map before destruction or was readded to map after being removed: 'Not Reached', file nsFrameManager.cpp, line 734
###!!! ASSERTION: wrong kind of child frame: 'aIsBlock == f->GetStyleDisplay()->IsBlockLevel()', file nsLineBox.cpp, line 70
Comment 40 Daniel Holbert [:dholbert] 2008-05-05 14:04:32 PDT
Created attachment 319455 [details]
backtrace of 'testcase' crash (using Branch / OS X)

Branch OS X Crash is:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xdddddddd
0x11fe728a in nsCSSFrameConstructor::FindFrameWithContent ()

Attaching backtrace.
Comment 41 Daniel Holbert [:dholbert] 2008-05-05 15:38:35 PDT
Created attachment 319468 [details] [diff] [review]
branch patch v2

Ok, this updated branch patch addresses the issues from comment 38:
 - Splits out nsTreeBoxObject class declaration into nsTreeBoxObject.h
 - Adds the (trivial) GetCachedTreeBody implementation, taken from Bug
397954's patch.

However, this sadly doesn't fix the segfault / assertions / warnings mentioned in comment 39 :(
Comment 42 Daniel Holbert [:dholbert] 2008-05-05 17:00:42 PDT
Created attachment 319481 [details]
backtrace of 'testcase' crash (using Branch / OS X) (detail)

Fixed my debug build a bit (per bug 401767 comment 14), and now I've got this more-detailed backtrace for the crash.
Comment 43 Daniel Holbert [:dholbert] 2008-05-05 17:08:09 PDT
Comment on attachment 319468 [details] [diff] [review]
branch patch v2

> NS_IMETHODIMP
> nsTreeColFrame::Init(nsPresContext*  aPresContext,
> ...
>-  InvalidateColumns();
>+  InvalidateColumns(PR_FALSE);
>   return rv;
> }
> 
> NS_IMETHODIMP                                                                   
> nsTreeColFrame::Destroy(nsPresContext* aPresContext)                          
> {
>   InvalidateColumns(); 

Aha -- it looks like applying the trunk patch with fuzz got me into trouble here -- the 'InvalidateColumns(PR_FALSE);' call ended up in Init(), when it actually belongs in Destroy().

I'm trying a build with that fixed right now.
Comment 44 Daniel Holbert [:dholbert] 2008-05-05 17:11:17 PDT
Created attachment 319485 [details] [diff] [review]
branch patch v3

This patch fixes the mistake mentioned in previous comment.  It does indeed fix the crash on 'testcase'.
Comment 45 Daniel Holbert [:dholbert] 2008-05-05 17:14:28 PDT
FWIW, after applying the patch, I still get the assertions and warnings mentioned in comment 39, except that this scary assertion....

> ###!!! ASSERTION: frame was not removed from primary frame map before
> destruction or was readded to map after being removed: 'Not Reached', file
> nsFrameManager.cpp, line 734

... is now gone, and replaced with this warning:

WARNING: cannot get parent, file /Users/dholbert/builds/branch/mozilla/widget/src/mac/nsWindow.cpp, line 2380
Comment 46 Daniel Holbert [:dholbert] 2008-05-05 17:23:38 PDT
Created attachment 319487 [details] [diff] [review]
branch patch v3b

This patch is identical to the last -- I just cleaned up some trailing whitespace in nsTreeColFrame::Destroy around the patched region.
Comment 47 Daniel Holbert [:dholbert] 2008-05-28 14:57:37 PDT
Comment on attachment 319487 [details] [diff] [review]
branch patch v3b

Thanks for the r+sr, roc!

Requesting approval for inclusion in 1.8.1.15.
Comment 48 Daniel Veditz [:dveditz] 2008-05-30 11:22:44 PDT
Comment on attachment 319487 [details] [diff] [review]
branch patch v3b

Approved for 1.8.1.15, a=dveditz for release-drivers
Comment 49 Daniel Holbert [:dholbert] 2008-05-30 13:31:22 PDT
Branch patch checked in.

Checking in src/nsBoxObject.h;
/cvsroot/mozilla/layout/xul/base/src/nsBoxObject.h,v  <--  nsBoxObject.h
new revision: 1.15.14.2; previous revision: 1.15.14.1
done
Checking in src/tree/src/nsTreeBoxObject.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBoxObject.cpp,v  <--  nsTreeBoxObject.cpp
new revision: 1.45.32.6; previous revision: 1.45.32.5
done
Checking in src/tree/src/nsTreeBoxObject.h;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBoxObject.h,v  <--  nsTreeBoxObject.h
new revision: 1.2.16.1; previous revision: 1.2
done
Checking in src/tree/src/nsTreeColFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeColFrame.cpp,v  <--  nsTreeColFrame.cpp
new revision: 1.32.24.2; previous revision: 1.32.24.1
done
Checking in src/tree/src/nsTreeColFrame.h;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeColFrame.h,v  <--  nsTreeColFrame.h
new revision: 1.19.24.2; previous revision: 1.19.24.1
done
Comment 50 Al Billings [:abillings] 2008-06-10 17:46:00 PDT
I can't repro the crash with either testcase using Windows XP or OS X 10.5.3. It just sits there in all instances.
Comment 51 Al Billings [:abillings] 2008-06-10 17:47:07 PDT
Oh, I forgot: This is specifically using 2.0.0.14 in order to verify the crash before verifying the fix for 2.0.0.15.
Comment 52 Daniel Holbert [:dholbert] 2008-06-10 17:55:13 PDT
IIRC, for branch, this only crashed in debug builds, and crashed on Mac but not on Linux.  (so, not sure if it affected Windows)
Comment 53 Al Billings [:abillings] 2008-06-10 17:59:12 PDT
Ah, good to know.

Do you have a debug branch build that you could verify this with? I think Tomcat builds debug for QA but just Windows.
Comment 54 Carsten Book [:Tomcat] 2008-06-10 18:07:40 PDT
(In reply to comment #53)
> Do you have a debug branch build that you could verify this with? I think
> Tomcat builds debug for QA but just Windows.
> 
Also Mac and Linux, but only trunk builds. But maybe martijn or bob should be also to verify this bug, i think they build also 1.8 branch builds.
Comment 55 Al Billings [:abillings] 2008-06-10 18:15:48 PDT
Martijn or Bob, can either of you verify this in a debug 1.8 mac build?
Comment 56 Bob Clary [:bc:] 2008-06-11 07:15:12 PDT
verified fixed 1.8.1.15 mac os x 10.5.3 no crash on either testcase.
Comment 57 Daniel Holbert [:dholbert] 2008-06-11 08:47:44 PDT
(In reply to comment #56)
> verified fixed 1.8.1.15 mac os x 10.5.3 no crash on either testcase.

Bob, Just to confirm -- did you test this in a debug build?  (see comment 52 thru 55)
Comment 58 Bob Clary [:bc:] 2008-06-11 10:28:37 PDT
yes, debug.
Comment 59 Daniel Holbert [:dholbert] 2009-03-11 17:31:25 PDT
I checked in "testcase" and "testcase2" (adding "reftest-wait", since they use JS & minor setTimeouts):  http://hg.mozilla.org/mozilla-central/rev/25a5167ee32d

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