Closed Bug 611242 Opened 9 years ago Closed 9 years ago

Webpages can modify the InstallTrigger prototype allowing all open webpages to share data and leaks until shutdown

Categories

(Toolkit :: Add-ons Manager, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jruderman, Assigned: mossop)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, testcase)

Attachments

(3 files, 1 obsolete file)

Attached file b.html
Steps:
1. Install 'DOM Fuzz Lite' from
    https://www.squarefree.com/extensions/domFuzzLite.xpi
2. Run a debug build of Firefox like this:
    XPCOM_MEM_LEAK_LOG=2 firefox b.html
3. Click the "Quit with leak check" button.

Result on Linux: "Windows: 8"
Expected on Linux: "Windows: 4"
Attached file baseline.html
An InstallTrigger object is created on each window, and each InstallTrigger has a reference to the window it is on. So each forms a cycle with its window.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions-content.js#52

I'm not sure though how a leak until shutdown could happen here - if nothing else refers to the window, shouldn't the cycle be GCed?
Component: DOM → Add-ons Manager
Product: Core → Toolkit
QA Contact: general → add-ons.manager
InstallTrigger.enabled.k is visible in other pages loaded into the *same tab*. This seems bad.
Er... yes.  How did that happen?
blocking2.0: --- → ?
InstallTrigger.enabled is a function on the InstallTrigger prototype which is I guess shared by all content windows, probably regardless of what tab they're in. Presumably you could just use InstallTrigger.prototype.mysharedvariable to share info across any webpages loaded at the same time.
blocking2.0: ? → betaN+
(In reply to comment #5)
> InstallTrigger.enabled is a function on the InstallTrigger prototype which is I
> guess shared by all content windows, probably regardless of what tab they're
> in. Presumably you could just use InstallTrigger.prototype.mysharedvariable to
> share info across any webpages loaded at the same time.

I lie, you can't get the prototype because it isn't exposed, but still this does allow for data sharing across all webpages. The same turns out to be true for the new console object.
Summary: Leak-until-shutdown with InstallTrigger → Webpages can modify the console and InstallTrigger prototypes allowing all open webpages to share data and leaks until shutdown
CC'd robcee and kdangoor so we can figure out how to help. Perhaps a mochitest case?
Ever since bug 610714 and bug 587734 landed, the web console has used an nsIDOMGlobalPropertyInitializer to create a new unique object for each page. With this in place is the console still affected by this?
yeah, I'd like a retest of the above to confirm that this is still a problem.
This doesn't apply to the console object since lazy console, but it still applies to InstallTrigger.
Summary: Webpages can modify the console and InstallTrigger prototypes allowing all open webpages to share data and leaks until shutdown → Webpages can modify the InstallTrigger prototype allowing all open webpages to share data and leaks until shutdown
Assignee: nobody → dtownsend
(In reply to comment #3)
> InstallTrigger.enabled.k is visible in other pages loaded into the *same tab*.
> This seems bad.

I wrote a test for this and then discovered I couldn't make it fail in the test or manually from the webconsole. Is this actually still a problem?
Yes, I can still reproduce that on trunk (rev 9aa42cdd6ab4).  You might see it as a security error when you should get undefined.
Attached patch patch rev 1 (obsolete) — Splinter Review
Ok this patch seems to solve the leaking shown by the testcase attached to the bug, I don't know how to automate that test. The patch also includes a testcase that verifies that setting InstallTrigger.enabled.foo doesn't make it appear in a different window but when I could reproduce that manually before I no longer can so maybe I'm missing something.
Comment on attachment 498247 [details] [diff] [review]
patch rev 1

Ok the information leak actually is only restricted to the single tab, I could have sworn it was all tabs before but anyway I have a good test for this now.
Attachment #498247 - Attachment is obsolete: true
Whiteboard: [waiting on try]
Attached patch patch rev 1Splinter Review
Frame scripts are loaded once per tab which means every tab gets its own InstallTrigger.prototype which is why information isn't being leaked across tabs, also why this leak goes away if you close the tab before quitting with the leak check. So this patch fixes both by doing away with the prototype and just creating a new object instance each time an InstallTrigger is used for a new window.

This is a diff with whitespace changes ignored since other than that it is fairly straightforward. I got rid of the need to walk the prototype chain to find the right window by just using the reference from the closure to get the right one straight away.
Attachment #502530 - Flags: review?(robert.bugzilla)
Whiteboard: [waiting on try] → [has patch][needs review rs]
Whiteboard: [has patch][needs review rs] → [has patch]
Landed: http://hg.mozilla.org/mozilla-central/rev/286f71fe6a2e
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b10
Verified fixed with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110210 Firefox/4.0b12pre and the steps in comment 0.

Windows: 4
Documents: 4
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.