Closed
Bug 611242
Opened 14 years ago
Closed 14 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)
Tracking
()
VERIFIED
FIXED
mozilla2.0b10
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: jruderman, Assigned: mossop)
Details
(Keywords: memory-leak, testcase)
Attachments
(3 files, 1 obsolete file)
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"
Reporter | ||
Comment 1•14 years ago
|
||
Comment 2•14 years ago
|
||
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
Reporter | ||
Comment 3•14 years ago
|
||
InstallTrigger.enabled.k is visible in other pages loaded into the *same tab*. This seems bad.
Assignee | ||
Comment 5•14 years ago
|
||
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+
Assignee | ||
Comment 6•14 years ago
|
||
(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
Comment 7•14 years ago
|
||
CC'd robcee and kdangoor so we can figure out how to help. Perhaps a mochitest case?
Comment 8•14 years ago
|
||
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•14 years ago
|
||
yeah, I'd like a retest of the above to confirm that this is still a problem.
Comment 10•14 years ago
|
||
This doesn't apply to the console object since lazy console, but it still applies to InstallTrigger.
Updated•14 years ago
|
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 | ||
Updated•14 years ago
|
Assignee: nobody → dtownsend
Assignee | ||
Comment 11•14 years ago
|
||
(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?
Reporter | ||
Comment 12•14 years ago
|
||
Yes, I can still reproduce that on trunk (rev 9aa42cdd6ab4). You might see it as a security error when you should get undefined.
Assignee | ||
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
Whiteboard: [waiting on try]
Assignee | ||
Comment 15•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [waiting on try] → [has patch][needs review rs]
Updated•14 years ago
|
Attachment #502530 -
Flags: review?(robert.bugzilla) → review+
Updated•14 years ago
|
Whiteboard: [has patch][needs review rs] → [has patch]
Assignee | ||
Comment 16•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b10
Comment 17•14 years ago
|
||
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.
Description
•