Closed Bug 742414 Opened 12 years ago Closed 12 years ago

Firefox crashes when viewing page source

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Version:
11 Branch
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla14
Tracking Flags:
Tracking Status
firefox11 --- wontfix
firefox12 --- wontfix
firefox13 --- verified
firefox14 --- verified
firefox-esr10 --- unaffected

People

(Reporter: fionov, Assigned: hsivonen)

References

(
URL
)

Details

(Keywords: crash, regression, reproducible, Whiteboard: [qa!])

Crash Data

Attachments

(2 files, 2 obsolete files)

Copy of URL provided in bug report
6.49 KB, text/html
Details
Fix bug 731234 and stop crashing as a side effect, with more lines hoisted into a function
14.38 KB, patch
smaug
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta-
Details | Diff | Splinter Review
Attached file funny.html (obsolete) — 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.113 Safari/535.19

Steps to reproduce:

Try to view source in the attached page by clicking page with right mouse button and choosing "View source". Page layout is similar to http://paste.lisp.org/display/128764 .


Actual results:

Firefox crashes (version 11.0 and above, any OS).


Expected results:

Firefox opens window with highlighted page source.
Sorry, User Agent is: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
URL: http://paste.lisp.org/display/128764
Actually, your funny.html doesn't crash it. But the website you provided does!
Severity: normal → critical
Status: UNCONFIRMED → NEW
status-firefox14: unaffected → affected
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Version: 11 Branch → Trunk

        Attachment #612229 -
        Attachment is obsolete: true

    
    

    
  
bp-dbeae445-6794-42e3-ad18-625d72120404
Crash Signature: [@ nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**)]
Component: Untriaged → HTML: Parser
Keywords: crash, 
          
            reproducible
Product: Firefox → Core
QA Contact: untriaged → parser

    
    

    
  
3 other crashes with crash signature nsHtml5Highlighter::CurrentNode():
bp-22899771-9800-48cb-bc72-9eef52120404
bp-11f2a0cb-b925-4131-b176-1c7a82120404
bp-94757ab2-4ea6-477e-bb9d-adb872120404

Unable to replicate it with Alice's crash signature.

    
    

    
  
Regression window(m-c),
Works:
http://hg.mozilla.org/mozilla-central/rev/6e219763ddd0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111101 Firefox/10.0a1 ID:20111101052615
Crash:
http://hg.mozilla.org/mozilla-central/rev/cd9add22f090
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111101 Firefox/10.0a1 ID:20111101073309
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6e219763ddd0&tochange=cd9add22f090


Regression window(m-i),
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/746e7c151de2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111101 Firefox/10.0a1 ID:20111101015315
Crash:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d9cc2539a85d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a1) Gecko/20111031 Firefox/10.0a1 ID:20111101043415
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=746e7c151de2&tochange=d9cc2539a85d

Suspected: Bug 482921
Blocks: 482921
Keywords: regression

    
    

    
  
I don't think it can be a regression when its the entire new view source parser.

    
    

    
  
(In reply to Jesper Hansen from comment #2)
> Actually, your funny.html doesn't crash it.

It was uploaded with wrong content/type, but still crashes it as html. Another example:

view-source:data:text/html,<!DOCTYPE html><html><script%0A></script%0A><script%0A></script%0A><script%0A></script%0A></html>

    
  
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED

    
  
Version: Trunk → 10 Branch

    
    

    
  
Comment on attachment 614726 [details] [diff] [review]
Fix bug 731234 and stop crashing as a side effect

Review note: You can use the test cases added here to gain confidence that the patch does something right. Other than that, you could look at the possible state transitions in the tokenizer and compare various end tag-like things with each other in the highlighter.

        Attachment #614726 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 614726 [details] [diff] [review]
Fix bug 731234 and stop crashing as a side effect

So, we have
EndCharacters();
StartSpan();
mCurrentRun = CurrentNode();
in 3 places now - in fact, each time EndCharacters() is used.
Could you perhaps rename EndCharacters to something more
descriptive and make it call StartSpan() and mCurrentRun = CurrentNode();

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 614726 [details] [diff] [review]
> Fix bug 731234 and stop crashing as a side effect
> 
> So, we have
> EndCharacters();
> StartSpan();
> mCurrentRun = CurrentNode();
> in 3 places now - in fact, each time EndCharacters() is used.
> Could you perhaps rename EndCharacters to something more
> descriptive and make it call StartSpan() and mCurrentRun = CurrentNode();

Would EndCharactersAndStartMarkupRun() work?

        Attachment #615653 -
        Flags: review?(bugs) → review+

    
    

    
  
Thanks for the review. Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc8a3e8c956
Flags: in-testsuite+
Target Milestone: --- → mozilla14

    
    

    
  
This doesn't happen on ESR, since ESR uses the old View Source highlighter.

          status-firefox-esr10:
          --- → unaffected
Version: 10 Branch → 11 Branch

    
    

    
  
Comment on attachment 615653 [details] [diff] [review]
Fix bug 731234 and stop crashing as a side effect, with more lines hoisted into a function

[Approval Request Comment]
Regression caused by (bug #): Bug 482921
User impact if declined: 1) Firefox crashes if the user chooses to View Source on a page that happens to contain a script end tag that has white space between the word "script" and the > sign. 2) A malicious site could crash Firefox using a view-source: URL that points to content with the aforementioned sort of script end tag. (No such malicious site seen in the wild, AFAIK.)
Testing completed (on m-c, etc.): The landing here contained thorough reftests that have passed without crashing on all platforms multiple times on mozilla-inbound. The code hasn't been in nightlies yet, since the code hasn't merged to mozilla-central yet. (I'm requesting approval now in case there's a chance of getting this crash fix in Firefox 12 still.)
Risk to taking this patch (and alternatives if risky): The fix here is contained to the View Source function of Firefox, so the worst case is trading one View Source crash to another (if this fix has a bug that hasn't been noticed).
String changes made by this patch: None.

        Attachment #615653 -
        Flags: approval-mozilla-beta?

        Attachment #615653 -
        Flags: approval-mozilla-aurora?

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/fdc8a3e8c956
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 615653 [details] [diff] [review]
Fix bug 731234 and stop crashing as a side effect, with more lines hoisted into a function

[Triage Comment]
Approving for Aurora 13. At this point in the FF12 cycle, we're open to only chemspill-worthy bugs. Since we didn't chemspill FF11, my expectation is that we would not do so for FF12. This will be resolved for our release channel users in 6 weeks.

        Attachment #615653 -
        Flags: approval-mozilla-beta?

        Attachment #615653 -
        Flags: approval-mozilla-beta-

        Attachment #615653 -
        Flags: approval-mozilla-aurora?

        Attachment #615653 -
        Flags: approval-mozilla-aurora+

    
    

    
  
(In reply to Alex Keybl [:akeybl] from comment #21)
> Approving for Aurora 13. 

Thanks. Landed:
https://hg.mozilla.org/releases/mozilla-aurora/rev/d44c15d3ecd0

> At this point in the FF12 cycle, we're open to only
> chemspill-worthy bugs. Since we didn't chemspill FF11, my expectation is
> that we would not do so for FF12.

OK. (While I agree this isn't chemspill-worthy, the fix was available & reviewed only yesterday, so there wasn't that much of opportunity to consider taking it in an 11 chemspill before considering whether to take it in 12 before release.)

          status-firefox12:
          affected → wontfix

          status-firefox13:
          affected → fixed

    
  
Whiteboard: [qa+]

    
    

    
  
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0

Verified the fix using the steps from bug description and no crash occured.
Marking verified on Firefox 13.

          status-firefox13:
          fixed → verified

    
    

    
  
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20100101 Firefox/14.0

Verified the fix with the steps from comment #0: no crash occurred when viewing page sources at the provided links.

Marking VERIFIED on Firefox 14 (beta 7).
Status: RESOLVED → VERIFIED

          status-firefox14:
          fixed → verified
Whiteboard: [qa+] → [qa!]

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: