Last Comment Bug 776323 - Crash with inserthtml, strikethrough on img
: Crash with inserthtml, strikethrough on img
Status: RESOLVED FIXED
: crash, regression, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 15 Branch
: All All
: -- critical (vote)
: mozilla17
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
: 778619 (view as bug list)
Depends on:
Blocks: 336383 CVE-2012-3959
  Show dependency treegraph
 
Reported: 2012-07-21 23:58 PDT by Jesse Ruderman
Modified: 2012-10-08 05:31 PDT (History)
8 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
fixed
16+
verified


Attachments
testcase (crashes Firefox when loaded) (404 bytes, text/html)
2012-07-21 23:58 PDT, Jesse Ruderman
no flags Details
stack trace (14.29 KB, text/plain)
2012-07-21 23:59 PDT, Jesse Ruderman
no flags Details
Patch part 1 -- Fix crash due to silly logic error in nsSelectionState::SaveSelection (2.09 KB, patch)
2012-07-22 03:12 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
bajaj.bhavana: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
Patch part 2 -- Clean up nsSelectionState::SaveSelection (35.02 KB, patch)
2012-07-22 03:15 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-07-21 23:58:33 PDT
Created attachment 644725 [details]
testcase (crashes Firefox when loaded)
Comment 1 Jesse Ruderman 2012-07-21 23:59:03 PDT
Created attachment 644726 [details]
stack trace

Nightly: bp-fcc805ed-36e7-47d7-93da-2765e2120722
Comment 2 Scoobidiver (away) 2012-07-22 00:15:18 PDT
On Windows: bp-72bb4d49-3eb8-44d2-8482-d64272120722.
Comment 3 :Aryeh Gregor (working until September 2) 2012-07-22 02:57:48 PDT
This was a stupid regression from bug 771994.  Easy to fix, though.
Comment 4 :Aryeh Gregor (working until September 2) 2012-07-22 03:12:48 PDT
Created attachment 644741 [details] [diff] [review]
Patch part 1 -- Fix crash due to silly logic error in nsSelectionState::SaveSelection

So I blame the fact that C for loops are used for both "repeat X times" and "do something to each thing in a range".  If we had a construct like "repeat(count)", with no unused index variable, this kind of bug would be impossible.

Also, I blame multi-range selections.  If selections had only one range, this kind of bug would also be impossible.  There would be no loop here to start with.

Alternatively, I should pay attention when I code rather than finding things to blame for my bugs, but that's less fun.


[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 771994
User impact if declined: Crash due to null pointer dereference, already observed in the wild.
Testing completed (on m-c, etc.): None yet.
Risk to taking this patch (and alternatives if risky): Low.  The fix is effectively one line.  The old code was just completely broken, and nobody noticed right away because we have lousy test coverage for multi-range selections.
String or UUID changes made by this patch: None.
Comment 5 :Aryeh Gregor (working until September 2) 2012-07-22 03:15:34 PDT
Created attachment 644742 [details] [diff] [review]
Patch part 2 -- Clean up nsSelectionState::SaveSelection

Cleanup patch after the actual bug fix this time, for ease of backporting.  This cleanup spilled over into requiring lots of de-COMtamination, just the way I like it!

Try: https://tbpl.mozilla.org/?tree=Try&rev=d08869e2b5c4
Comment 8 :Ehsan Akhgari 2012-07-26 19:35:04 PDT
Comment on attachment 644741 [details] [diff] [review]
Patch part 1 -- Fix crash due to silly logic error in nsSelectionState::SaveSelection

This is needed on beta as well.
Comment 9 Alex Keybl [:akeybl] 2012-07-27 17:13:17 PDT
(In reply to :Aryeh Gregor from comment #4)
> Alternatively, I should pay attention when I code rather than finding things
> to blame for my bugs, but that's less fun.

:)

> [Approval Request Comment]
> Risk to taking this patch (and alternatives if risky): Low.  The fix is
> effectively one line.  The old code was just completely broken, and nobody
> noticed right away because we have lousy test coverage for multi-range
> selections.

Sounds good, let's get this on branches.
Comment 11 Simona B [:simonab ] -PTO- back Sept 5th 2012-08-03 07:16:35 PDT
Verified that Firefox 15 beta 3 does not crash when loading the test case from the Description.

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.6:

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

Verified also the Socorro reports and could not find any crashes that occurred after the patches landed.
Comment 12 :Aryeh Gregor (working until September 2) 2012-08-12 10:58:55 PDT
*** Bug 778619 has been marked as a duplicate of this bug. ***
Comment 13 Scoobidiver (away) 2012-09-08 07:09:57 PDT
It's #3 top browser crasher in 10.0.7esr.
Did bug 771994 land in this version?
Comment 14 :Ehsan Akhgari 2012-10-02 10:51:01 PDT
Comment on attachment 644741 [details] [diff] [review]
Patch part 1 -- Fix crash due to silly logic error in nsSelectionState::SaveSelection

See the risk assessment for the previous branch approval requests.  The short version is that this is pretty safe.
Comment 15 bhavana bajaj [:bajaj] 2012-10-02 11:08:50 PDT
Comment on attachment 644741 [details] [diff] [review]
Patch part 1 -- Fix crash due to silly logic error in nsSelectionState::SaveSelection

Approving the patch as it is low risk and needed for ESR .
Comment 17 Simona B [:simonab ] -PTO- back Sept 5th 2012-10-08 05:31:45 PDT
Verified that Firefox 10.0.8 ESR does not crash when loading the test case from the Description.

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.7:

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

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