887499 - Crash after GC makes HTMLOptionsCollection outlive its HTMLSelectElement

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Turns out that v1 crashed all over:

https://tbpl.mozilla.org/?tree=Try&rev=4b94243819a7

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 769641 [details] [diff] [review]
Patch v2

Sounds like we should null check mOptions somewhere, not leave it out from unlink.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

This looks better.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v4

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/3c88309f82ad