776323 - Crash with inserthtml, strikethrough on img

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Created attachment 644725 [details]
testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Created attachment 644726 [details]
stack trace

Nightly: bp-fcc805ed-36e7-47d7-93da-2765e2120722

Comment 2

[Scoobidiver (away)]

On Windows: bp-72bb4d49-3eb8-44d2-8482-d64272120722.

Comment 3

[Aryeh Gregor (:ayg) (next working August 2-22)]

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

Comment 4

[Aryeh Gregor (:ayg) (next working August 2-22)]

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) (next working August 2-22)]

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) (next working August 2-22)]

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

Comment 7

[Ed Morley [:emorley]]

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

Comment 8

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

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]]

(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 (needinfo please, extremely long backlog)]

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

Comment 11

[Simona Badau [Away. Please needinfo? Alin Deac [:alindeac]]]

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 13

[Scoobidiver (away)]

It's #3 top browser crasher in 10.0.7esr.
Did bug 771994 land in this version?

Comment 14

[:Ehsan Akhgari (needinfo please, extremely long backlog)]

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]]

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 (needinfo please, extremely long backlog)]

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

Comment 17

[Simona Badau [Away. Please needinfo? Alin Deac [:alindeac]]]

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