Last Comment Bug 742414 - Firefox crashes when viewing page source
: Firefox crashes when viewing page source
Status: VERIFIED FIXED
[qa!]
: crash, regression, reproducible
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: 11 Branch
: All All
: -- critical (vote)
: mozilla14
Assigned To: Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
:
Mentors:
http://paste.lisp.org/display/128764
: 743208 746323 750888 (view as bug list)
Depends on:
Blocks: 482921
  Show dependency treegraph
 
Reported: 2012-04-04 09:29 PDT by Sergey Fionov
Modified: 2012-06-19 04:56 PDT (History)
12 users (show)
hsivonen: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
verified
verified
unaffected


Attachments
funny.html (118 bytes, text/plain)
2012-04-04 09:29 PDT, Sergey Fionov
no flags Details
Copy of URL provided in bug report (6.49 KB, text/html)
2012-04-04 09:41 PDT, Jesper Hansen
no flags Details
Fix bug 731234 and stop crashing as a side effect (12.42 KB, patch)
2012-04-13 04:07 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
no flags Details | Diff | Splinter Review
Fix bug 731234 and stop crashing as a side effect, with more lines hoisted into a function (14.38 KB, patch)
2012-04-17 04:08 PDT, Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01)
bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Sergey Fionov 2012-04-04 09:29:42 PDT
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.
Comment 1 Sergey Fionov 2012-04-04 09:35:21 PDT
Sorry, User Agent is: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Comment 2 Jesper Hansen 2012-04-04 09:37:09 PDT
Actually, your funny.html doesn't crash it. But the website you provided does!
Comment 3 Jesper Hansen 2012-04-04 09:41:15 PDT
Created attachment 612231 [details]
Copy of URL provided in bug report
Comment 4 Alice0775 White 2012-04-04 09:44:25 PDT
bp-dbeae445-6794-42e3-ad18-625d72120404
Comment 5 Jesper Hansen 2012-04-04 09:47:11 PDT
I am getting bp-ded00165-b440-4c8d-a541-9332c2120404
Comment 6 Jesper Hansen 2012-04-04 09:54:57 PDT
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 Alice0775 White 2012-04-04 09:56:44 PDT
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
Comment 8 Jesper Hansen 2012-04-04 10:20:20 PDT
I don't think it can be a regression when its the entire new view source parser.
Comment 9 Sergey Fionov 2012-04-04 22:30:44 PDT
(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>
Comment 10 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-10 01:11:38 PDT
*** Bug 743208 has been marked as a duplicate of this bug. ***
Comment 11 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-13 04:07:13 PDT
Created attachment 614726 [details] [diff] [review]
Fix bug 731234 and stop crashing as a side effect
Comment 12 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-13 11:48:21 PDT
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.
Comment 13 Olli Pettay [:smaug] 2012-04-17 03:24:29 PDT
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();
Comment 14 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-17 03:39:33 PDT
(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?
Comment 15 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-17 04:08:32 PDT
Created attachment 615653 [details] [diff] [review]
Fix bug 731234 and stop crashing as a side effect, with more lines hoisted into a function
Comment 16 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-17 04:55:39 PDT
Thanks for the review. Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc8a3e8c956
Comment 17 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-17 05:10:40 PDT
This doesn't happen on ESR, since ESR uses the old View Source highlighter.
Comment 18 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-17 22:43:51 PDT
*** Bug 746323 has been marked as a duplicate of this bug. ***
Comment 19 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-17 23:32:25 PDT
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.
Comment 20 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-18 05:23:06 PDT
https://hg.mozilla.org/mozilla-central/rev/fdc8a3e8c956
Comment 21 Alex Keybl [:akeybl] 2012-04-18 15:18:02 PDT
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.
Comment 22 Henri Sivonen (:hsivonen) (Not doing reviews or reading bugmail until 2016-08-01) 2012-04-18 23:52:45 PDT
(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.)
Comment 23 Scoobidiver (away) 2012-05-01 22:04:05 PDT
*** Bug 750888 has been marked as a duplicate of this bug. ***
Comment 24 Mihaela Velimiroviciu (:mihaelav) 2012-05-09 03:36:06 PDT
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.
Comment 25 Mihaela Velimiroviciu (:mihaelav) 2012-06-19 04:56:38 PDT
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).

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