Closed Bug 635329 Opened 12 years ago Closed 12 years ago

Crash [@ nsBidiPresUtils::RemoveBidiContinuation] with large margin, changing text node

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: smontagu)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:dos (critical w/out frame-poisoning)] [softblocker][has patch])

Crash Data

Attachments

(3 files, 1 obsolete file)

testcase (crashes Firefox when loaded)
364 bytes, text/html
Details
stack trace (mac)
16.14 KB, text/plain
Details
Addressed comments
9.83 KB, patch
roc
: review+
roc
: approval2.0+
Details | Diff | Splinter Review 
      No description provided.
Nominating for blocking because this might be the same as a topcrash with the same signature (bug 632828).
blocking2.0: --- → ?
Attached file stack trace (mac) — 

    
  
OS: Windows 7 → All
Assignee: nobody → smontagu
blocking2.0: ? → final+
Whiteboard: [sg:dos (critical w/out frame-poisoning)] → [sg:dos (critical w/out frame-poisoning)] [softblocker]

    
    

    
  
This turns out to be caused by bug 597627 deleting frames during bidi resolution. I think that we won't be losing much if we just don't do that optimization when SetLength is called from AdjustOffsetsForBidi, since bidi resolution blows away text runs for all frames it touches anyway. Mats, does that make sense?
Depends on: 597627

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #513850 -
        Flags: review?(roc)

        Attachment #513850 -
        Flags: feedback?(matspal)

    
  
Whiteboard: [sg:dos (critical w/out frame-poisoning)] [softblocker] → [sg:dos (critical w/out frame-poisoning)] [softblocker][has patch]
Blocks: 597627
No longer depends on: 597627
Keywords: regression

    
    

    
  
Comment on attachment 513850 [details] [diff] [review]
Patch

Yes, this looks good to me.
Nit: prefix the parameter with "a"

        Attachment #513850 -
        Flags: feedback?(matspal) → feedback+

    
    

    
  
Comment on attachment 513850 [details] [diff] [review]
Patch

+                 PRBool inBidiResolution = PR_FALSE);

I think you should just have a flags word with a flag "ALLOW_FRAME_CREATION_DESTRUCTION", and the default (0) should not allow it.

    
    

    
  


  

        Attachment #513850 -
        Attachment is obsolete: true

        Attachment #513896 -
        Flags: review?(roc)

        Attachment #513850 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 513896 [details] [diff] [review]
Addressed comments

+#define ALLOW_FRAME_CREATION_DESTRUCTION 1

Hmm, might as well make it ALLOW_FRAME_CREATION_AND_DESTRUCTION :-). Also, make it 0x01. Also, should be an enum in nsTextFrame.h next to SetLength, i.e.
  enum { ALLOW_FRAME_CREATION_AND_DESTRUCTION = 0x01 };

        Attachment #513896 -
        Flags: review?(roc) → review+

    
    

    
  
Comment on attachment 513896 [details] [diff] [review]
Addressed comments

This should be very safe since it just enables selective non-use of the optimizations added in bug 597627.

        Attachment #513896 -
        Flags: approval2.0+

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/3db6693e136e
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

    
  
Blocks: 633476
Crash Signature: [@ nsBidiPresUtils::RemoveBidiContinuation]

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