Last Comment Bug 712733 - "Long URL Please" add-on creates zombie compartments on pages that have links which the add-on supports
: "Long URL Please" add-on creates zombie compartments on pages that have links...
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks: LeakyAddons ZombieCompartments
  Show dependency treegraph
 
Reported: 2011-12-21 12:13 PST by Fanolian
Modified: 2012-02-28 08:38 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
about:memory?verbose (12.04 KB, text/plain)
2011-12-21 12:13 PST, Fanolian
no flags Details

Description Fanolian 2011-12-21 12:13:23 PST
Created attachment 583578 [details]
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/.
Comment 1 Fanolian 2011-12-21 12:15:22 PST
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
Comment 2 Justin Lebar (not reading bugmail) 2011-12-21 12:58:57 PST
I e-mailed the extension author.
Comment 3 Nicholas Nethercote [:njn] 2011-12-21 14:37:28 PST
Fanolian, thanks for all your great bug reports lately!
Comment 4 Fanolian 2011-12-21 16:02:52 PST
(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. :)
Comment 5 Nicholas Nethercote [:njn] 2012-01-03 20:56:56 PST
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.
Comment 6 darragh 2012-01-05 04:20:15 PST
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.
Comment 7 Nicholas Nethercote [:njn] 2012-01-05 04:37:49 PST
CC'ing bz, who's good at this sort of thing :)
Comment 8 Justin Lebar (not reading bugmail) 2012-01-05 05:52:47 PST
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.
Comment 9 Nicholas Nethercote [:njn] 2012-01-05 06:00:43 PST
So you probably want to release the reference when the page unload event occurs.
Comment 10 Justin Lebar (not reading bugmail) 2012-01-05 06:09:00 PST
...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);
Comment 11 darragh 2012-01-05 06:38:49 PST
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?
Comment 12 Justin Lebar (not reading bugmail) 2012-01-05 06:54:37 PST
> 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.
Comment 13 Fanolian 2012-01-05 08:12:17 PST
(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.
Comment 14 darragh 2012-01-05 13:38:49 PST
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?
Comment 15 Justin Lebar (not reading bugmail) 2012-01-05 14:05:34 PST
> 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?
Comment 16 Fanolian 2012-01-20 12:38:36 PST
(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.
Comment 17 Jorge Villalobos [:jorgev] 2012-02-08 14:51:05 PST
(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.
Comment 18 Nicholas Nethercote [:njn] 2012-02-08 15:01:00 PST
> 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 :)
Comment 19 Jorge Villalobos [:jorgev] 2012-02-08 15:31:38 PST
(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.
Comment 20 Kris Maglione [:kmag] 2012-02-15 17:00:22 PST
(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>
Comment 21 Nicholas Nethercote [:njn] 2012-02-15 17:18:34 PST
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 :)
Comment 22 Nicholas Nethercote [:njn] 2012-02-15 17:44:02 PST
> 
> 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.
Comment 23 Kris Maglione [:kmag] 2012-02-15 17:55:46 PST
(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.
Comment 24 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-15 18:28:47 PST
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.
Comment 25 Jorge Villalobos [:jorgev] 2012-02-22 13:17:44 PST
@darragh: did you read comment #17? Your latest version still leaks and I indicated what needs fixing.
Comment 26 darragh 2012-02-25 15:19:26 PST
@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.
Comment 27 Leszek Zyczkowski 2012-02-25 15:37:37 PST
I tested version 0.5.1 and confirm - no memory leaks.

Note You need to log in before you can comment on or make changes to this bug.