Closed Bug 712733 Opened 13 years ago Closed 13 years ago

"Long URL Please" add-on creates zombie compartments on pages that have links which the add-on supports

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: Fanolian+BMO, Unassigned)

References

Details

(Whiteboard: [MemShrink:P3])

Attachments

(1 file)

about:memory?verbose
12.04 KB, text/plain
Details
Attached file about:memory?verbose —
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20111221 Firefox/12.0a1 Build ID: 20111221031226 Steps to reproduce: It is an addon that "replaces short urls with the originals so you can see where links actually link to". 1. In a new profile, install the latest Long URL Please (0.4.4) at https://addons.mozilla.org/en-US/firefox/addon/long-url-please/. 2. Go to a page that contains shortened URLs, or create a local simple testcase: <html> <body> <a href="http://tinyurl.com/1c2"> http://tinyurl.com/1c2</a> </body> </html> (The link redirects to http://www.google.com/.) 3. Open the testcase in a new tab in Firefox and then close it. There is no need to click or hover on the link. 4. Open about:memory?verbose and click "Minimize memory usage" a dozen times. Actual results: The compartment of the testcase is not destroyed. If a page does not contain any links that the addon supports, the page does not become a zombie compartment upon closing it. I had reported the issue to the addon author at https://code.google.com/p/longurlplease-firefox/issues/detail?id=35 but there has been no feedback for over 2 weeks. Therefore I report it here as well as a record. P.S. Long URL Please Mod 0.4.6, which is based on Long URL Please, has the same issue as well. This addon can be found at https://addons.mozilla.org/en-US/firefox/addon/long-url-please-mod/.
The attachment is displaying gibberish again... I paste it here instead: Main Process Explicit Allocations 25,151,715 B (100.0%) -- explicit ├──10,942,026 B (43.50%) -- js │ ├───6,174,022 B (24.55%) -- compartment([System Principal], 0x7e72000) │ │ ├──3,506,176 B (13.94%) -- gc-heap │ │ │ ├──1,014,160 B (04.03%) -- shapes │ │ │ │ ├────638,376 B (02.54%) -- tree │ │ │ │ ├────217,984 B (00.87%) -- base │ │ │ │ └────157,800 B (00.63%) -- dict │ │ │ ├────933,968 B (03.71%) -- objects │ │ │ │ ├──683,216 B (02.72%) -- function │ │ │ │ └──250,752 B (01.00%) -- non-function │ │ │ ├────790,032 B (03.14%) -- arena │ │ │ │ ├──762,568 B (03.03%) -- unused │ │ │ │ ├───13,768 B (00.05%) -- padding │ │ │ │ └───13,696 B (00.05%) -- headers │ │ │ ├────720,288 B (02.86%) -- scripts │ │ │ ├─────28,800 B (00.11%) -- type-objects │ │ │ ├─────18,720 B (00.07%) -- strings │ │ │ └────────208 B (00.00%) -- xml │ │ ├──1,174,232 B (04.67%) -- script-data │ │ ├────580,904 B (02.31%) -- shapes-extra │ │ │ ├──245,768 B (00.98%) -- compartment-tables │ │ │ ├──185,728 B (00.74%) -- tree-tables │ │ │ ├───86,976 B (00.35%) -- tree-shape-kids │ │ │ └───62,432 B (00.25%) -- dict-tables │ │ ├────375,360 B (01.49%) -- object-slots │ │ ├────327,680 B (01.30%) -- mjit-code │ │ ├────131,072 B (00.52%) -- analysis-temporary │ │ ├─────50,230 B (00.20%) -- string-chars │ │ ├─────17,104 B (00.07%) -- type-inference │ │ │ └──17,104 B (00.07%) -- script-main │ │ └─────11,264 B (00.04%) -- mjit-data │ ├───2,093,056 B (08.32%) -- gc-heap-decommitted │ ├───1,101,444 B (04.38%) -- compartment(atoms) │ │ ├────630,368 B (02.51%) -- string-chars │ │ ├────471,040 B (01.87%) -- gc-heap │ │ │ ├──460,368 B (01.83%) -- strings │ │ │ └───10,672 B (00.04%) -- arena │ │ │ ├───8,048 B (00.03%) -- unused │ │ │ ├───1,840 B (00.01%) -- headers │ │ │ └─────784 B (00.00%) -- padding │ │ └─────────36 B (00.00%) -- shapes-extra │ │ └──36 B (00.00%) -- compartment-tables │ ├───1,009,216 B (04.01%) -- runtime │ │ ├────573,440 B (02.28%) -- threads │ │ │ ├──262,144 B (01.04%) -- stack-committed │ │ │ ├──180,224 B (00.72%) -- normal │ │ │ ├──131,072 B (00.52%) -- regexp-code │ │ │ └────────0 B (00.00%) -- temporary │ │ ├────262,144 B (01.04%) -- atoms-table │ │ ├────151,552 B (00.60%) -- runtime-object │ │ └─────22,080 B (00.09%) -- contexts │ ├─────318,944 B (01.27%) -- xpconnect │ ├─────115,696 B (00.46%) -- compartment(file:///F:/TinyURL.html) │ │ ├───94,208 B (00.37%) -- gc-heap │ │ │ ├──51,320 B (00.20%) -- arena │ │ │ │ ├──50,696 B (00.20%) -- unused │ │ │ │ ├─────368 B (00.00%) -- headers │ │ │ │ └─────256 B (00.00%) -- padding │ │ │ ├──27,160 B (00.11%) -- shapes │ │ │ │ ├──15,432 B (00.06%) -- dict │ │ │ │ ├───6,784 B (00.03%) -- base │ │ │ │ └───4,944 B (00.02%) -- tree │ │ │ ├──14,384 B (00.06%) -- objects │ │ │ │ ├──12,512 B (00.05%) -- function │ │ │ │ └───1,872 B (00.01%) -- non-function │ │ │ ├───1,056 B (00.00%) -- type-objects │ │ │ └─────288 B (00.00%) -- scripts │ │ ├───13,792 B (00.05%) -- shapes-extra │ │ │ ├───6,656 B (00.03%) -- dict-tables │ │ │ ├───6,528 B (00.03%) -- compartment-tables │ │ │ ├─────320 B (00.00%) -- tree-shape-kids │ │ │ └─────288 B (00.00%) -- tree-tables │ │ ├────7,680 B (00.03%) -- object-slots │ │ └───────16 B (00.00%) -- script-data │ ├──────98,304 B (00.39%) -- gc-heap-chunk-admin │ ├──────31,344 B (00.12%) -- compartment(moz-nullprincipal:{54a3e43a- a7b3-476d-91e1-423b60d1985f}) │ │ ├──28,672 B (00.11%) -- gc-heap │ │ │ ├──25,048 B (00.10%) -- arena │ │ │ │ ├──24,792 B (00.10%) -- unused │ │ │ │ ├─────144 B (00.00%) -- padding │ │ │ │ └─────112 B (00.00%) -- headers │ │ │ ├───1,848 B (00.01%) -- shapes │ │ │ │ ├────984 B (00.00%) -- tree │ │ │ │ ├────480 B (00.00%) -- base │ │ │ │ └────384 B (00.00%) -- dict │ │ │ ├───1,504 B (00.01%) -- objects │ │ │ │ ├──1,248 B (00.00%) -- function │ │ │ │ └────256 B (00.00%) -- non-function │ │ │ ├─────144 B (00.00%) -- scripts │ │ │ └─────128 B (00.00%) -- type-objects │ │ ├───1,536 B (00.01%) -- object-slots │ │ ├───1,128 B (00.00%) -- shapes-extra │ │ │ ├────904 B (00.00%) -- compartment-tables │ │ │ ├────160 B (00.00%) -- dict-tables │ │ │ └─────64 B (00.00%) -- tree-shape-kids │ │ └───────8 B (00.00%) -- script-data │ ├───────────0 B (00.00%) -- gc-heap-chunk-dirty-unused │ └───────────0 B (00.00%) -- gc-heap-chunk-clean-unused ├───6,777,487 B (26.95%) -- heap-unclassified ├───4,452,792 B (17.70%) -- storage │ ├──3,895,688 B (15.49%) -- sqlite │ │ ├──1,482,984 B (05.90%) -- places.sqlite │ │ │ ├──1,319,352 B (05.25%) -- cache-used [3] │ │ │ ├────127,376 B (00.51%) -- stmt-used [3] │ │ │ └─────36,256 B (00.14%) -- schema-used [3] │ │ ├────961,976 B (03.82%) -- other │ │ ├────394,176 B (01.57%) -- chromeappsstore.sqlite │ │ │ ├──330,192 B (01.31%) -- cache-used │ │ │ ├───59,824 B (00.24%) -- stmt-used │ │ │ └────4,160 B (00.02%) -- schema-used │ │ ├────262,560 B (01.04%) -- webappsstore.sqlite │ │ │ ├──198,576 B (00.79%) -- cache-used │ │ │ ├───59,824 B (00.24%) -- stmt-used │ │ │ └────4,160 B (00.02%) -- schema-used │ │ ├────170,424 B (00.68%) -- cookies.sqlite │ │ │ ├──165,408 B (00.66%) -- cache-used │ │ │ ├────3,120 B (00.01%) -- stmt-used │ │ │ └────1,896 B (00.01%) -- schema-used │ │ ├────161,864 B (00.64%) -- urlclassifier3.sqlite │ │ │ ├───92,416 B (00.37%) -- stmt-used │ │ │ ├───66,696 B (00.27%) -- cache-used │ │ │ └────2,752 B (00.01%) -- schema-used │ │ ├────144,024 B (00.57%) -- content-prefs.sqlite │ │ │ ├──132,504 B (00.53%) -- cache-used │ │ │ ├────9,008 B (00.04%) -- stmt-used │ │ │ └────2,512 B (00.01%) -- schema-used │ │ ├────108,264 B (00.43%) -- downloads.sqlite │ │ │ ├───99,600 B (00.40%) -- cache-used │ │ │ ├────6,832 B (00.03%) -- stmt-used │ │ │ └────1,832 B (00.01%) -- schema-used │ │ ├────106,784 B (00.42%) -- permissions.sqlite │ │ │ ├───99,600 B (00.40%) -- cache-used │ │ │ ├────5,888 B (00.02%) -- stmt-used │ │ │ └────1,296 B (00.01%) -- schema-used │ │ └────102,632 B (00.41%) -- search.sqlite │ │ ├───99,600 B (00.40%) -- cache-used │ │ ├────1,792 B (00.01%) -- stmt-used │ │ └────1,240 B (00.00%) -- schema-used │ └────557,104 B (02.21%) -- prefixset │ └──557,104 B (02.21%) -- all ├───1,075,824 B (04.28%) -- layout │ ├────520,208 B (02.07%) -- shell (chrome://browser/content/browser.xul) │ │ ├──322,352 B (01.28%) -- arenas │ │ └──197,856 B (00.79%) -- styledata │ ├────373,024 B (01.48%) -- shell(about:memory?verbose) │ │ ├──312,112 B (01.24%) -- arenas │ │ └───60,912 B (00.24%) -- styledata │ ├─────94,080 B (00.37%) -- shell(file:///F:/TinyURL.html) │ │ ├──61,520 B (00.24%) -- styledata │ │ └──32,560 B (00.13%) -- arenas │ └─────88,512 B (00.35%) -- shell(resource://gre- resources/hiddenWindow.html) │ ├──58,000 B (00.23%) -- styledata │ └──30,512 B (00.12%) -- arenas ├─────955,424 B (03.80%) -- xpti-working-set ├─────592,051 B (02.35%) -- startup-cache │ ├──591,651 B (02.35%) -- mapping │ └──────400 B (00.00%) -- data ├─────180,798 B (00.72%) -- images │ ├──176,688 B (00.70%) -- chrome │ │ ├──176,688 B (00.70%) -- used │ │ │ ├──176,688 B (00.70%) -- uncompressed-heap │ │ │ ├────────0 B (00.00%) -- raw │ │ │ └────────0 B (00.00%) -- uncompressed-nonheap │ │ └────────0 B (00.00%) -- unused │ │ ├──0 B (00.00%) -- raw │ │ ├──0 B (00.00%) -- uncompressed-heap │ │ └──0 B (00.00%) -- uncompressed-nonheap │ └────4,110 B (00.02%) -- content │ ├──4,110 B (00.02%) -- used │ │ ├──3,216 B (00.01%) -- uncompressed-heap │ │ ├────894 B (00.00%) -- raw │ │ └──────0 B (00.00%) -- uncompressed-nonheap │ └──────0 B (00.00%) -- unused │ ├──0 B (00.00%) -- raw │ ├──0 B (00.00%) -- uncompressed-heap │ └──0 B (00.00%) -- uncompressed-nonheap ├─────154,021 B (00.61%) -- dom │ └──154,021 B (00.61%) -- window-objects │ └──154,021 B (00.61%) -- active │ ├──142,151 B (00.57%) -- top=1 (inner=2) │ │ ├──139,129 B (00.55%) -- inner-window(id=2, uri=chrome://browser/content/browser.xul) │ │ ├────1,511 B (00.01%) -- inner-window(id=10, uri=about:blank) │ │ └────1,511 B (00.01%) -- inner-window(id=9, uri=about:blank) │ ├────5,663 B (00.02%) -- top=7 (inner=18) │ │ └──5,663 B (00.02%) -- inner-window(id=18, uri=about:memory?verbose) │ ├────2,400 B (00.01%) -- outer-windows [6] │ ├────2,049 B (00.01%) -- top=14 (inner=17) │ │ └──2,049 B (00.01%) -- inner-window(id=17, uri=file:///F:/TinyURL.html) │ └────1,758 B (00.01%) -- top=3 (inner=4) │ └──1,758 B (00.01%) -- inner-window(id=4, uri=resource://gre-resources/hiddenWindow.html) ├──────20,604 B (00.08%) -- cycle-collector ├─────────496 B (00.00%) -- history-links-hashtable ├─────────192 B (00.00%) -- gfx │ └──192 B (00.00%) -- textrun-word-cache └───────────0 B (00.00%) -- spell-check Other Measurements 105,120 B -- gfx-d2d-surfacecache 7,482,180 B -- gfx-d2d-surfacevram 190,060 B -- gfx-surface-image 0 B -- gfx-surface-win32 17,547,712 B -- heap-allocated 22,855,680 B -- heap-committed 23.19% -- heap-committed-fragmentation 3,559,424 B -- heap-dirty 17,054,462 B -- heap-unallocated 2 -- js-compartments-system 2 -- js-compartments-user 6,291,456 B -- js-gc-heap 846,104 B -- js-gc-heap-arena-unused 0 B -- js-gc-heap-chunk-clean-unused 0 B -- js-gc-heap-chunk-dirty-unused 2,093,056 B -- js-gc-heap-decommitted 46.71% -- js-gc-heap-unused-fraction 131,072 B -- js-total-analysis-temporary 338,944 B -- js-total-mjit 1,334,432 B -- js-total-objects 1,894,976 B -- js-total-scripts 1,551,668 B -- js-total-shapes 1,159,686 B -- js-total-strings 47,088 B -- js-total-type-inference 0 -- low-memory-events-physical 0 -- low-memory-events-virtual 74,326,016 B -- private 95,764,480 B -- resident 472,903,680 B -- vsize
Blocks: ZombieCompartments, LeakyAddons
Whiteboard: [MemShrink]
I e-mailed the extension author.
Fanolian, thanks for all your great bug reports lately!
(In reply to Nicholas Nethercote [:njn] from comment #3) > Fanolian, thanks for all your great bug reports lately! All kudos to your detail introduction and instructions from https://developer.mozilla.org/en/Zombie_Compartments. It really helps and guides common users like me to provide meaningful information for the developers. :)
Whiteboard: [MemShrink] → [MemShrink:P3]
I can reproduce this on the test case in comment 0, but I can't get the add-on to work with t.co links on Twitter. I think only one zombie compartment is kept alive at once -- when another page with a shortened URL is loaded, the previous zombie dies -- but I'm not certain.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Addon Long URL Please creates zombie compartments on pages that have links which the addon supports → "Long URL Please" add-on creates zombie compartments on pages that have links which the add-on supports
I'm sorry to have caused this. Are there any obvious causes for this in the plugin source? (< 200 LOC) http://code.google.com/p/longurlplease-firefox/source/browse/trunk/src/content/longurlplease.js I appreciate your help on this.
CC'ing bz, who's good at this sort of thing :)
Well, there's this: longurlplease.document = aEvent.originalTarget; You store a reference to the document, but don't get rid of it until something else loads.
So you probably want to release the reference when the page unload event occurs.
...or just don't store the document in a global to begin with, which is probably what you want in order to make this add-on work properly with multiple tabs. (Right now it looks like it'll only properly act on DOMNodeInserted events which occur in the most-recently loaded tab.) Note that (I think) you could store the document in a callback which gets killed when the document dies. That is, you could do something like var doc = <the content document>; longurlplease.document.addEventListener("DOMNodeInserted", function() { longurlplease.rerunAfterDomChange(doc); }, true);
Ok. Thanks, I suspected longurlplease.document but was confused by: 'If a page does not contain any links that the addon supports, the page does not become a zombie compartment upon closing it.' Any explanation for this?
> Any explanation for this? Have you observed this behavior as well? It could be just a mistake in testing -- the tester loaded a page without any shortened links, then loaded a page with shortened links, noticed that there was a zombie compartment only for the latter page, and concluded that this was because the former page had no shortened links, rather than because it wasn't the last-loaded page.
(In reply to Nicholas Nethercote [:njn] from comment #5) > I can reproduce this on the test case in comment 0, but I can't get the > add-on to work with t.co links on Twitter. I think only one zombie > compartment is kept alive at once -- when another page with a shortened URL > is loaded, the previous zombie dies -- but I'm not certain. After some testings my layman guess is that the extension does not work on Twitter, and Google+, at all. I find a page with t.co links, but with many other links as well, in Wordpress[1] which becomes a zombie compartment after closing it. Putting a t.co link in a local testcase causes zombie compartment too.
Hrmm. I cannot reproduce (mac os x firefox 6.0.2). But I simplified the js removing the longurlplease.document bit and the rescanning of the dom after dom changes. I think this file should be accessible: https://addons.mozilla.org/firefox/downloads/file/140796/long_url_please-0.5.0-fx.xpi?src=devhub Can you confirm the changes fix the issue?
> Hrmm. I cannot reproduce (mac os x firefox 6.0.2). That's a really old version of Firefox. Can you please test with Firefox 9 or newer?
(In reply to darragh from comment #14) > Hrmm. I cannot reproduce (mac os x firefox 6.0.2). > > But I simplified the js removing the longurlplease.document bit and the > rescanning of the dom after dom changes. > > I think this file should be accessible: > > https://addons.mozilla.org/firefox/downloads/file/140796/long_url_please-0.5. > 0-fx.xpi?src=devhub > > Can you confirm the changes fix the issue? Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120120 Firefox/12.0a1 I can still reproduce a zombie compartment using 3 different pages: the simple testcase described in comment#0, this bugzilla page (it contains tinyurl in comment#0), and http://firefoxnightly.tumblr.com/post/16118462126/errors-from-before-the-web-console-is-opened-are which contains a t.co link.
(In reply to darragh from comment #14) > Can you confirm the changes fix the issue? Function replaceUsingLongUrlPleaseAPI is leaking both aTag and aTags because you are not declaring them. That may be the (other) source for the leak. BTW, the editor who reviewed your previous update was wrong about the evals (we allow that type of usage), so you can ignore that comment. I'm keeping that update in preliminary review because of the leak, though.
> Function replaceUsingLongUrlPleaseAPI is leaking both aTag and aTags because > you are not declaring them. That may be the (other) source for the leak. Would "use strict;" have caught this? Maybe that should be mandatory for add-ons :)
(In reply to Nicholas Nethercote [:njn] from comment #18) > Would "use strict;" have caught this? Maybe. > Maybe that should be mandatory for add-ons :) We try to impose as little as possible. I also know there are some limitations when using "use strict;" that would make it more difficult to create add-ons with it.
(In reply to Nicholas Nethercote [:njn] from comment #18) > Would "use strict;" have caught this? Maybe that should be mandatory for > add-ons :) There are unfortunately some problems with this. I'd like to use "use strict". I did use it from its inception until a few months ago. Unfortunately, it's becoming a minefield. Had it been fully specified and implemented as such before it was pushed into production, it would make code less likely to fail. The core devs rather often decide that new things should be illegal in strict mode that are not illegal elsewhere and break add-on that use those features. The latest was E4X, which can no longer be used in strict mode at all. There's every reason to suspect that other Gecko-specific features, like array comprehensions and generator functions (which unlike E4X, are not ECMA standards) may be outlawed at some point as well, if they ever fall out of favor with the JS engine team. And of course there are add-ons like Firebug and Firebug-related add-ons which make vociferous use of the with statement, which is illegal in strict mode, and can't easily be refactored to remove it. There are other traps as well. Popular extensions like Tab Mix Plus, for instance, tends to check arguments.callee.caller.caller to decide whether it's being called by its own code. Add-ons in strict mode which try to call gBrowser.addTab will explode when used with Tab Mix Plus, and as any add-on developer knows, conflicts with Tab Mix Plus cause an endless flood of hate mail. If there were a way to choose features of strict mode that didn't break add-ons to bits and were guaranteed not to punish add-ons for using them in the future, I'd be all for requiring it. As that doesn't seem likely to happen, though, it's just not practicable. </rant>
Fair enough. But if you're writing a new add-on and/or not relying on those features like e4x and |with|, it's a clear win, IMO :)
> > There are other traps as well. Popular extensions like Tab Mix Plus, for > instance, tends to check arguments.callee.caller.caller to decide whether > it's being called by its own code. Add-ons in strict mode which try to call > gBrowser.addTab will explode when used with Tab Mix Plus, and as any add-on > developer knows, conflicts with Tab Mix Plus cause an endless flood of hate > mail. Oh, that sounds bad. How does that happen? I thought strict mode only applied to the file in question.
(In reply to Nicholas Nethercote [:njn] from comment #21) > Fair enough. But if you're writing a new add-on and/or not relying on those > features like e4x and |with|, it's a clear win, IMO :) I don't think that's clear, no. If strict mode were static, I think an argument could be made. But it's not. New bans are added to strict mode fairly frequently, which makes it dangerous for anyone hoping for long term compatibility. I also don't think that E4X is something that should be avoided. It's the safest, clearest, and simplest way that I know to generate XML from JavaScript, and it's currently the only way to include multi-line literals in JavaScript files without escapes being processed. Before it was banned in strict mode, I was actively encouraging developers to use it more. (In reply to Nicholas Nethercote [:njn] from comment #22) > Oh, that sounds bad. How does that happen? I thought strict mode only > applied to the file in question. The caller property can't be accessed on any strict mode function. I'm not entirely sure that the caller property can be accessed when the caller is a strict mode function either, but I think it can.
The caller property of a function with strict mode code has always been inaccessible. ECMAScript 5 mandates this. There's nothing we could do about this even if we wanted to do something. Tab Mix Plus, for better or worse, shouldn't be using the call stack that way, if it can be called by code that might not be strict mode code. And given what it can do to performance, there's *possibly* a case that we should start watching for it when reviewing AMO submissions and telling authors that they should start moving away from it. But that's a policy concern I'm only mentioning because it occurs to me, not because I think it might happen, will happen, or even should happen. E4X is its own can of worms, but I think you overstate things to say that we might start banning other things in strict mode. It's particularly worth noting that many of those other things (array comprehensions and generators in particular are, I believe) are on the standards track now. E4X is emphatically not so, which makes it a significantly different matter.
@darragh: did you read comment #17? Your latest version still leaks and I indicated what needs fixing.
@jorgev, Thanks for the prompt. Your suggestion in #17 was spot on. I could reproduce before applying, but not after. Thanks for the help. Version 0.5.1 is in the approval queue: https://addons.mozilla.org/en-US/developers/addon/long-url-please/versions/1318226 Sorry again for the hassle.
I tested version 0.5.1 and confirm - no memory leaks.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: