726332 - nsFormFillController's MutationObserver handling is suspicious

Status: RESOLVED FIXED

(Toolkit :: Autocomplete, defect)

Description

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

nsFormFillController uses MutationObserver for input.list, but nothing, 
if I read the code correctly, enforces that MutationObserver is actually removed.
Everything works usually ok, but if list element gets moved to some other document
and modified there, there could be some sg:crit crashes, I think.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

nsFormFillController::StopControllingInput (called on blur and pagehide of the input) seems to call RemoveMutationObserver(). Is that not sufficient?

Comment 2

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

That doesn't help at all. The element input.list returns may not be there anymore.

Comment 3

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

This will be fixed as a part of Bug 726334.

Comment 4

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

The patch in bug 726334 guarantees that a deleted formfillcontroller is never a mutationobserver
of a live dom node.

Comment 5

[Daniel Veditz [:dveditz]]

Marking sg:critical assuming comment 0 meant web content could do that. And if that's the case we probably need to land bug 726334 and bug 730470 on ESR10.

Comment 6

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

Some more testing would be great, but I'll prepare patches for ESR10

Comment 7

[u279076]

What can QA do to verify this fix?

Comment 8

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

Unfortunately there isn't any testcase for this.
One could create one if needed: the test case would first have input.list set, and then
remove the element to which input.list points to.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Olli Pettay [:smaug] from comment #6)
> Some more testing would be great, but I'll prepare patches for ESR10

Hey Olli, how are the patches coming for ESR? It would be great to get some approvals/landings for this soon.

Comment 10

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

Attachment: patch for bug 726334 updated for esr10

Comment 11

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

Attachment: patch for bug Bug 730470 updated for esr10

Comment 12

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

Attachment: both

Both patches needed just some minor merging.

Comment 13

[Justin Dolske [:Dolske]]

Should this bug have a specific reviewer or is it meant to dangle in the wind? Mounir, perhaps?

Comment 14

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

Comment on attachment 615040 [details] [diff] [review]
both

Oops, I thought I had asked gavin to review this.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 615040 [details] [diff] [review]
both

Approved to land, as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process

Comment 16

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

https://hg.mozilla.org/releases/mozilla-esr10/rev/ddec781cfc63