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 (:ayg) (away until October 25) 2012-07-22 02:57:48 PDT

This was a stupid regression from bug 771994.  Easy to fix, though.

Comment 4 Aryeh Gregor (:ayg) (away until October 25) 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 (:ayg) (away until October 25) 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 6 Aryeh Gregor (:ayg) (away until October 25) 2012-07-24 01:25:55 PDT

https://hg.mozilla.org/integration/mozilla-inbound/rev/12f5569247f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/51a7bf2b46f1

Comment 7 Ed Morley [:emorley] 2012-07-25 08:13:50 PDT

https://hg.mozilla.org/mozilla-central/rev/12f5569247f0
https://hg.mozilla.org/mozilla-central/rev/51a7bf2b46f1

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 10 :Ehsan Akhgari 2012-07-30 10:51:37 PDT

http://hg.mozilla.org/releases/mozilla-aurora/rev/ab0ab9e65f96
http://hg.mozilla.org/releases/mozilla-beta/rev/80000a1e4ea0

Comment 11 Simona B [:simonab ] 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 (:ayg) (away until October 25) 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 16 :Ehsan Akhgari 2012-10-02 11:15:09 PDT

https://hg.mozilla.org/releases/mozilla-esr10/rev/29a475b5686b

Comment 17 Simona B [:simonab ] 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