nsFormFillController's MutationObserver handling is suspicious

RESOLVED FIXED

Status

()

Toolkit
Autocomplete
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

12 Branch
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox12 fixed, firefox-esr1012+ fixed)

Details

(Whiteboard: [sg:critical][qa-] fixed in 726334, 730470)

Attachments

(3 attachments)

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.
nsFormFillController::StopControllingInput (called on blur and pagehide of the input) seems to call RemoveMutationObserver(). Is that not sufficient?
That doesn't help at all. The element input.list returns may not be there anymore.
This will be fixed as a part of Bug 726334.
The patch in bug 726334 guarantees that a deleted formfillcontroller is never a mutationobserver
of a live dom node.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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.
status-firefox-esr10: --- → affected
status-firefox12: --- → fixed
tracking-firefox-esr10: --- → ?
Whiteboard: [sg:critical] fixed in 726334, 730470
Some more testing would be great, but I'll prepare patches for ESR10

Updated

6 years ago
tracking-firefox-esr10: ? → 12+
What can QA do to verify this fix?
Whiteboard: [sg:critical] fixed in 726334, 730470 → [sg:critical][qa?] fixed in 726334, 730470
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.
Whiteboard: [sg:critical][qa?] fixed in 726334, 730470 → [sg:critical][qa-] fixed in 726334, 730470
(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.
Created attachment 615035 [details] [diff] [review]
patch for bug 726334 updated for esr10
Created attachment 615039 [details] [diff] [review]
patch for bug Bug 730470 updated for esr10
Created attachment 615040 [details] [diff] [review]
both

Both patches needed just some minor merging.
Assignee: nobody → bugs
Attachment #615040 - Flags: review?
Attachment #615040 - Flags: approval-mozilla-esr10?
Should this bug have a specific reviewer or is it meant to dangle in the wind? Mounir, perhaps?
Comment on attachment 615040 [details] [diff] [review]
both

Oops, I thought I had asked gavin to review this.
Attachment #615040 - Flags: review? → review?(gavin.sharp)
Attachment #615040 - Flags: review?(gavin.sharp) → review+
Comment on attachment 615040 [details] [diff] [review]
both

Approved to land, as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Attachment #615040 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
https://hg.mozilla.org/releases/mozilla-esr10/rev/ddec781cfc63
status-firefox-esr10: affected → fixed
Group: core-security
You need to log in before you can comment on or make changes to this bug.