Closed Bug 726332 Opened 8 years ago Closed 8 years ago

nsFormFillController's MutationObserver handling is suspicious


(Toolkit :: Autocomplete, defect)

12 Branch
Not set



Tracking Status
firefox12 --- fixed
firefox-esr10 12+ fixed


(Reporter: smaug, Assigned: smaug)


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


(3 files)

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.
Closed: 8 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.
Whiteboard: [sg:critical] fixed in 726334, 730470
Some more testing would be great, but I'll prepare patches for ESR10
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.
Attached patch bothSplinter Review
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]

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

Approved to land, as per
Attachment #615040 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Group: core-security
You need to log in before you can comment on or make changes to this bug.