Closed Bug 726332 Opened 12 years ago Closed 12 years ago

nsFormFillController's MutationObserver handling is suspicious

Categories

(Toolkit :: Autocomplete, defect)

Product:
Toolkit
Component:
Autocomplete
Version:
12 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox12 --- fixed
firefox-esr10 12+ fixed

People

(Reporter: smaug, Assigned: smaug)

Details

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

Attachments

(3 files)

patch for bug 726334 updated for esr10
15.16 KB, patch
Details | Diff | Splinter Review
patch for bug Bug 730470 updated for esr10
5.12 KB, patch
Details | Diff | Splinter Review
both
15.58 KB, patch
Gavin
: review+
lsblakk
: approval-mozilla-esr10+
Details | Diff | Splinter Review 
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
Closed: 12 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
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 both — — Splinter 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]
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+
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: