Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Crash with inserthtml, strikethrough on img

RESOLVED FIXED in Firefox 15

Status

()

Core
Editor
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: ayg)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

15 Branch
mozilla17
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15+ verified, firefox16+ fixed, firefox-esr1016+ verified)

Details

(crash signature)

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
Created attachment 644725 [details]
testcase (crashes Firefox when loaded)
(Reporter)

Comment 1

5 years ago
Created attachment 644726 [details]
stack trace

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

Comment 2

5 years ago
On Windows: bp-72bb4d49-3eb8-44d2-8482-d64272120722.
Crash Signature: [@ nsCOMPtr_base::begin_assignment | nsSelectionState::SaveSelection ] [@ nsRangeStore::StoreRange | nsSelectionState::SaveSelection] → [@ nsCOMPtr_base::begin_assignment | nsSelectionState::SaveSelection ] [@ nsRangeStore::StoreRange | nsSelectionState::SaveSelection] [@ nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) | nsRangeStore::StoreRange(nsIDOMRange*)]
OS: Mac OS X → All
Hardware: x86_64 → All
This was a stupid regression from bug 771994.  Easy to fix, though.
Assignee: nobody → ayg
Blocks: 771994
Status: NEW → ASSIGNED
status-firefox16: --- → affected
status-firefox17: --- → affected
Keywords: regression

Updated

5 years ago
Version: Trunk → 16 Branch
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.
Attachment #644741 - Flags: review?(ehsan)
Attachment #644741 - Flags: approval-mozilla-aurora?
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
Attachment #644742 - Flags: review?(ehsan)
Attachment #644741 - Flags: review?(ehsan) → review+
Attachment #644742 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f5569247f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/51a7bf2b46f1
Flags: in-testsuite+
Target Milestone: --- → mozilla17

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/12f5569247f0
https://hg.mozilla.org/mozilla-central/rev/51a7bf2b46f1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox17: affected → ---
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.
Attachment #644741 - Flags: approval-mozilla-beta?
tracking-firefox16: --- → +

Comment 9

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

Updated

5 years ago
status-firefox15: --- → affected
tracking-firefox15: --- → +

Updated

5 years ago
Attachment #644741 - Flags: approval-mozilla-beta?
Attachment #644741 - Flags: approval-mozilla-beta+
Attachment #644741 - Flags: approval-mozilla-aurora?
Attachment #644741 - Flags: approval-mozilla-aurora+
http://hg.mozilla.org/releases/mozilla-aurora/rev/ab0ab9e65f96
http://hg.mozilla.org/releases/mozilla-beta/rev/80000a1e4ea0
status-firefox15: affected → fixed
status-firefox16: affected → fixed
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.
status-firefox15: fixed → verified
Duplicate of this bug: 778619

Comment 13

5 years ago
It's #3 top browser crasher in 10.0.7esr.
Did bug 771994 land in this version?
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → ?
Version: 16 Branch → 15 Branch

Updated

5 years ago
tracking-firefox-esr10: ? → 16+
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.
Attachment #644741 - Flags: approval-mozilla-esr10?
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 .
Attachment #644741 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
https://hg.mozilla.org/releases/mozilla-esr10/rev/29a475b5686b
status-firefox-esr10: affected → fixed
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
status-firefox-esr10: fixed → verified
You need to log in before you can comment on or make changes to this bug.