Closed Bug 399694 Opened 17 years ago Closed 16 years ago

Crash [@ PresShell::GetPrimaryFrameFor] with contenteditable

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9beta5

People

(Reporter: jruderman, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical][dbaron-1.9:RsCc] post-1.8-branch)

Crash Data

Attachments

(5 files, 2 obsolete files)

testcase
423 bytes, text/html
Details
v1
762 bytes, patch
Details | Diff | Splinter Review
stack trace (mac debug, gdb)
7.13 KB, text/plain
Details
Trace + stack + frame dumps
19.10 KB, text/html
Details
Different patch, rev. 3
6.35 KB, patch
dbaron
: review+
dbaron
: superreview+
damons
: approval1.9+
Details | Diff | Splinter Review
Attached file testcase — 
Loading the testcase often triggers a crash with PresShell::GetPrimaryFrameFor calling a random address.
Flags: blocking1.9?
Only crashes when the testcase is local (and even then requires 2-3 tries).
Whiteboard: [sg:critical]
Flags: blocking1.9? → blocking1.9+
Whiteboard: [sg:critical] → [sg:critical][dbaron-1.9:RsCc]
FWIW, I can't reproduce this on Windows. I also get different results when I view the testcase in IE7, FF2, Safari, and Opera9.
Now I get a crash if I load the testcase from Bugzilla and press Cmd+R (reload).
I couldn't reproduce this with the nightly build [Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007101304 Minefield/3.0a9pre], and neither could I reproduce it with [Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9a9pre) Gecko/2007110414 Minefield/3.0a9pre]. Do you still get it Jesse?
Still crashes for me.
I see the crash on Mac if I reload from Bugzilla several times quickly.
Priority: -- → P3
Assignee: nobody → peterv
Priority: P3 → P2
Worksforme, using:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008012104 Minefield/3.0b3pre
Works for me on:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4pre) Gecko/2008020804 Minefield/3.0b4pre

Jesse, are you still able to reproduce on trunk?
Yes.  It took about 20 reloads this time.
We have this as a P2 blocker for Gecko 1.9.  Peter have you been able to make any progress on this one?
Attached patch v1 — — Splinter Review 
I'm also having a hard time reproducing this. I do hit an assertion in nsEditor (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/editor/libeditor/base/nsEditor.cpp&rev=1.505&mark=4167#4160). This patch fixes the assertion and is something we should fix in any case. Jesse, could you give this one a try?
The patch does not fix the crash for me.

Ping me on IRC sometime and I can get the crash in gdb for you, if that would help.
Well, a stack trace would help for sure.
Flags: wanted-next+
Flags: blocking1.9-
Flags: tracking1.9+

    
    

    
  
Peter, I have a fix for this so hope you don't mind me taking it...
Assignee: peterv → nobody
Component: Editor → Layout
OS: Mac OS X → All
QA Contact: editor → layout
Hardware: PC → All

    
  
Assignee: nobody → mats.palmgren

    
    

    
  


  
The problem is anonymous content nodes that are owned by frames.
If we have queued style changes for such content and there is
another style change that causes the frame tree to be destroyed
including the frame that owns that content there's a risk of a
dangling content reference in the style change list in
ProcessRestyledFrames() in case there are no other strong refs
other than from frames (similar to the situation in bug 420439).

Sequence of events in the attached trace:
1.  we ProcessOneRestyle for <body>, this leads to two changes
    for ProcessRestyledFrames() to process.
2.  when processing the first one (for content=0x1a044d0 (blue)),
    we decide we need to reframe the containing block <body>.
3+4 this destroys nsTextControlFrame@0x1a01170 (yellow), and its
    child frames and nsTextControlFrame calls DestroyAnonymousContent():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsTextControlFrame.cpp&rev=3.284&root=/cvsroot&mark=1172#1160
    since these frames were the last strong refs...
5.  ... the <br> content (the _moz_editor_bogus_node) is destroyed

Then we use the deleted 'changeData->mContent' here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1468&root=/cvsroot&mark=10049#9969

    
    

    
  
Hmm...  I swear we've run into this before.  That time the fix was to not (incorrectly) restyle the native anonymous content.  Why exactly are we ending up with a restyle on that node anyway?

Perhaps we just need to hold strong refs in this list, though...  David, thoughts?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Different patch, rev. 1 (obsolete)
        — 
          — Splinter Review
      

    


  
I considered holding strong refs only for DEBUG but I think it
would better to not take any chances here, we may end up in
RecreateFramesForContent():
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1468&root=/cvsroot&mark=11262,11268#11215
and other methods that assumes that destroying the frame doesn't
also destroy the content.

It's easy to detect if this happens BTW, in case we want to assert
something when it does, I tried:

PRBool anon = changeData->mContent->IsNativeAnonymous();
PRInt32 refcnt = (const_cast<nsIContent*>(changeData->mContent))->Release();
NS_ASSERTION(refcnt != 0 || anon,
             "frame was the last owner of its non-anonymous content");

but it turned out the _moz_editor_bogus_node isn't IsNativeAnonymous (?).

(the first hunk fixes a compile warning: unused variable 'tag' ...)

        Attachment #308095 -
        Flags: superreview?(bzbarsky)

        Attachment #308095 -
        Flags: review?(bzbarsky)

    
    

    
  
(In reply to comment #17)
> Why exactly are we ending up with a restyle on that node anyway?

The trace says change=0x5 (SyncFrameView+RepaintFrame) if that gives any clues...
I'll try to track down the exact cause.

    
    

    
  
> but it turned out the _moz_editor_bogus_node isn't IsNativeAnonymous (?).

Only the root node of an anonymous subtree is.  That's why we have the other methods for checking whether a node is in an anonymous subtree.

I think David would be a better reviewer for this, and I worry about performance with this approach...

    
  

        Attachment #308095 -
        Flags: superreview?(dbaron)

        Attachment #308095 -
        Flags: superreview?(bzbarsky)

        Attachment #308095 -
        Flags: review?(dbaron)

        Attachment #308095 -
        Flags: review?(bzbarsky)

    
    

    
  
Why not make nsStyleChangeList manage the reference counting instead?  Other than that, this seems fine (although I also don't see why the const_cast is needed; you should be able to call release on an nsIContent *const just fine (you'd only need a const_cast for a const nsIContent*).

        Attachment #308095 -
        Flags: superreview?(dbaron)

        Attachment #308095 -
        Flags: superreview-

        Attachment #308095 -
        Flags: review?(dbaron)

        Attachment #308095 -
        Flags: review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Different patch, rev. 2 (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to comment #21)
> Why not make nsStyleChangeList manage the reference counting instead?

Ok.  It does slightly more ref-counting than the first patch though.

> I also don't see why the const_cast is needed

Left over from an earlier version of the patch, sorry.

        Attachment #308095 -
        Attachment is obsolete: true

        Attachment #308525 -
        Flags: superreview?(dbaron)

        Attachment #308525 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 308525 [details] [diff] [review]
Different patch, rev. 2

> nsStyleChangeList::Clear() 
> {
>+  PRInt32 index = mCount;
>+  while (0 < index--) {

I'd prefer

for (PRInt32 index = mCount - 1; index >= 0; --index) {

>Index: layout/base/nsStyleChangeList.h
>+// Note: nsStyleChangeList holds a strong ref on the nsIContent
>+// while on the list.  Copying out values using ChangeAt()
>+// does not AddRef though.

This is a little misleading since one ChangeAt doesn't copy at all; it returns a pointer to the list.  So I'd say:

  Note:  nsStyleChangeList owns a reference to
   nsIContent pointers in its list.

Then above the first ChangeAt, say:

  Fills in pointers without reference counting.  

And then above the second ChangeAt:

  Fills in a pointer to the list entry storage (no reference counting involved).


r+sr=dbaron

        Attachment #308525 -
        Flags: superreview?(dbaron)

        Attachment #308525 -
        Flags: superreview+

        Attachment #308525 -
        Flags: review?(dbaron)

        Attachment #308525 -
        Flags: review+

    
    

    
  


  
Ok, but then I'd prefer to have same iteration style elsewhere in this file.

        Attachment #308525 -
        Attachment is obsolete: true

        Attachment #308541 -
        Flags: superreview?(dbaron)

        Attachment #308541 -
        Flags: review?(dbaron)

        Attachment #308541 -
        Flags: superreview?(dbaron)

        Attachment #308541 -
        Flags: superreview+

        Attachment #308541 -
        Flags: review?(dbaron)

        Attachment #308541 -
        Flags: review+

    
  

        Attachment #308541 -
        Flags: approval1.9?

    
    

    
  
Let's get this landed ASAP, please.

    
    

    
  
Comment on attachment 308541 [details] [diff] [review]
Different patch, rev. 3

a1.9+=damons

        Attachment #308541 -
        Flags: approval1.9? → approval1.9+

    
    

    
  
mozilla/layout/base/nsStyleChangeList.cpp 	1.21
mozilla/layout/base/nsStyleChangeList.h 	3.11
mozilla/layout/base/nsCSSFrameConstructor.cpp 	1.1469 

First perf stats from Linux & MacOSX Tinderboxes looks good (no change).

-> FIXED
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5

    
  
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical][dbaron-1.9:RsCc] → [sg:critical][dbaron-1.9:RsCc] post-1.8-branch
Group: core-security

    
    

    
  
Crashtest: http://hg.mozilla.org/mozilla-central/rev/43339764467b
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ PresShell::GetPrimaryFrameFor]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: