Firefox crashes when viewing page source

VERIFIED FIXED in Firefox 13

Status

()

Core
HTML: Parser
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Sergey Fionov, Assigned: hsivonen)

Tracking

({crash, regression, reproducible})

11 Branch
mozilla14
crash, regression, reproducible
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11 wontfix, firefox12 wontfix, firefox13 verified, firefox14 verified, firefox-esr10 unaffected)

Details

(Whiteboard: [qa!], crash signature, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 612229 [details]
funny.html

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.

Updated

5 years ago
status-firefox14: --- → unaffected
(Reporter)

Comment 1

5 years ago
Sorry, User Agent is: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0

Comment 2

5 years ago
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

Comment 3

5 years ago
Created attachment 612231 [details]
Copy of URL provided in bug report
Attachment #612229 - Attachment is obsolete: true

Comment 4

5 years ago
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

Comment 5

5 years ago
I am getting bp-ded00165-b440-4c8d-a541-9332c2120404

Comment 6

5 years ago
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.

Comment 7

5 years ago
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

Comment 8

5 years ago
I don't think it can be a regression when its the entire new view source parser.
(Reporter)

Comment 9

5 years ago
(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)

Updated

5 years ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 743208

Updated

5 years ago
Version: Trunk → 10 Branch
(Assignee)

Comment 11

5 years ago
Created attachment 614726 [details] [diff] [review]
Fix bug 731234 and stop crashing as a side effect
(Assignee)

Comment 12

5 years ago
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();
(Assignee)

Comment 14

5 years ago
(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?
(Assignee)

Comment 15

5 years ago
Created attachment 615653 [details] [diff] [review]
Fix bug 731234 and stop crashing as a side effect, with more lines hoisted into a function
Attachment #614726 - Attachment is obsolete: true
Attachment #614726 - Flags: review?(bugs)
Attachment #615653 - Flags: review?(bugs)

Updated

5 years ago
Attachment #615653 - Flags: review?(bugs) → review+
(Assignee)

Comment 16

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

Comment 17

5 years ago
This doesn't happen on ESR, since ESR uses the old View Source highlighter.
status-firefox-esr10: --- → unaffected
Version: 10 Branch → 11 Branch
(Assignee)

Updated

5 years ago
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
(Assignee)

Updated

5 years ago
Duplicate of this bug: 746323
(Assignee)

Comment 19

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
status-firefox11: affected → wontfix
status-firefox14: affected → 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+
(Assignee)

Comment 22

5 years ago
(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+]

Updated

5 years ago
Duplicate of this bug: 750888
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.