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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Jesse Ruderman]

Attachment: 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"

Comment 1

[Jesse Ruderman]

Attachment: baseline.html

Comment 2

[Alon Zakai (:azakai)]

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?

Comment 3

[Jesse Ruderman]

InstallTrigger.enabled.k is visible in other pages loaded into the *same tab*. This seems bad.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er... yes.  How did that happen?

Comment 5

[Dave Townsend [:mossop]]

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.

Comment 6

[Dave Townsend [:mossop]]

(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.

Comment 7

[David Dahl :ddahl]

CC'd robcee and kdangoor so we can figure out how to help. Perhaps a mochitest case?

Comment 8

[David Dahl :ddahl]

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?

Comment 9

[Rob Campbell [:rc] (:robcee)]

yeah, I'd like a retest of the above to confirm that this is still a problem.

Comment 10

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

This doesn't apply to the console object since lazy console, but it still applies to InstallTrigger.

Comment 11

[Dave Townsend [:mossop]]

(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?

Comment 12

[Jesse Ruderman]

Yes, I can still reproduce that on trunk (rev 9aa42cdd6ab4).  You might see it as a security error when you should get undefined.

Comment 13

[Dave Townsend [:mossop]]

Attachment: patch rev 1

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 14

[Dave Townsend [:mossop]]

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.

Comment 15

[Dave Townsend [:mossop]]

Attachment: patch rev 1

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.

Comment 16

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/286f71fe6a2e

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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