Crash after GC makes HTMLOptionsCollection outlive its HTMLSelectElement

RESOLVED FIXED in mozilla26

Status

()

--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jruderman, Assigned: Ms2ger)

Tracking

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

Trunk
mozilla26
x86_64
Mac OS X
crash, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 768003 [details]
testcase

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase

Result: Crash 

#5  0x00000001026454f1 in mozilla::dom::HTMLOptionsCollection::Add 

359	  mSelect->Add(aElement, aBefore, aError);

(gdb) p mSelect
$1 = (Cannot access memory at address 0x0
(Reporter)

Comment 1

5 years ago
Created attachment 768005 [details]
stack
(Assignee)

Comment 2

5 years ago
This isn't s-s, I think; the select nulls out the collection's pointer. We just need to make it a strong CC'd reference. I can do that later.
(Reporter)

Updated

5 years ago
Group: core-security
(Assignee)

Comment 3

5 years ago
Created attachment 768832 [details] [diff] [review]
Patch v1
Assignee: nobody → Ms2ger
Status: NEW → ASSIGNED
Attachment #768832 - Flags: review?(bugs)
Comment on attachment 768832 [details] [diff] [review]
Patch v1

Technically there should be a null check in HTMLOptionsCollection::Add, but in practice we should be safe even without it.
Attachment #768832 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 769641 [details] [diff] [review]
Patch v2

Turns out that v1 crashed all over:

https://tbpl.mozilla.org/?tree=Try&rev=4b94243819a7
Attachment #768832 - Attachment is obsolete: true
Attachment #769641 - Flags: review?(bugs)
Comment on attachment 769641 [details] [diff] [review]
Patch v2

Sounds like we should null check mOptions somewhere, not leave it out from unlink.
Attachment #769641 - Flags: review?(bugs) → review-
(Assignee)

Comment 7

5 years ago
Created attachment 770120 [details] [diff] [review]
Patch v3

This looks better.
Attachment #769641 - Attachment is obsolete: true
Attachment #770120 - Flags: review?(bugs)
Comment on attachment 770120 [details] [diff] [review]
Patch v3

mOptions handling is just assuming it is not null. So, I think we just need to add null checks to optionscollection. Sorry about not realizing that before.
Attachment #770120 - Flags: review?(bugs) → review-
(Assignee)

Updated

5 years ago
Assignee: Ms2ger → nobody
(Assignee)

Comment 9

5 years ago
Created attachment 788642 [details] [diff] [review]
Patch v4
Assignee: nobody → Ms2ger
Attachment #770120 - Attachment is obsolete: true
Attachment #788642 - Flags: review?(bugs)
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3c88309f82ad
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.