780391 - Wallflower Addon creates huge non-window-global compartment when running EcmaScript Test262

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Till Schneidereit [:till]]

Attachment: about:memory?verbose output after running 2463 tests from chapter 15

With the Wallflower 1.2 Addon installed, running the EcmaScript Test262 causes ridiculous memory usage.

(Specifically, I tried running the tests of chapter 15.)

The attached about:memory log is from a clean profile in yesterday's Nightly, with just the Wallflower addon installed.

Large amounts of memory are used for analysis-temporary and unused-gc-things, and the unused percentage of js-main-runtime-gc-heap-committed is pretty high: 42.16%.

Minimizing memory doesn't change anything, and continuing to run the test suite, memory usage consistently keeps climbing.

After closing the test suite's window, memory goes down to ~124MB after hitting Minimize Memory twice.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, it sounds like the addon is leaking and causing lots of fragmentation. But the analysis-temporary thing is very odd.

Comment 2

[Kris Maglione [:kmag]]

This uses an old version of Jetpack (1.6.1) that is known to leak in recent versions of Firefox, as I recall.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So it looks like jetpack is going off the rails here.  There are 2466 sandboxes for this one domain.  That's really broken.

Comment 4

[Nicholas Nethercote [inactive]]

Dietrich, the first thing to try here is to repack Wallflower with the latest version of JetPack and see if that fixes the problem.  Can you do that?  Thanks!

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

SDK 1.9 shipped today. It should be pretty leak resistant compared to 1.6.

Comment 6

[Kris Maglione [:kmag]]

I warned via AMO that the add-on will be downgraded if it isn't rebuilt with a newer Jetpack.

Comment 7

[Kris Maglione [:kmag]]

Dietrich updated the add-on to SDK 1.9 and we're still getting zombie compartments on this test, and seriously degraded performance as well. The add-on isn't doing anything too crazy, so it seems like a Jetpack bug:

main.js:
// Wallflower
const pm = require('page-mod');
pm.PageMod({
  include: '*',
  contentScriptWhen: 'start',
  contentScriptFile: require('self').data.url('wallflower.js')
});

wallflower.js:
(function killIframes() {
  var defaultURL = 'about:blank';
  if (inIframe() && (
    window.location.href.indexOf('facebook.com/extern/login_status') != -1 ||
    window.location.href.indexOf('plusone.google.com/u/0/_/+1/fastbutton') != -1 ||
    window.location.href.indexOf('plusone.google.com/_/+1/fastbutton') != -1 ||
    window.location.href.indexOf('plusone.google.com/_/+1/hover') != -1 ||
    window.location.href.indexOf('plus.google.com/u/0/_/notificati') != -1 ||
    window.location.href.indexOf('facebook.com/plugins/like.php') != -1 ||
    window.location.href.indexOf('fbcdn.net/connect') != -1))
  {
    window.location.href = defaultURL;
  }
})()
 
function inIframe() {
  return window.location != window.parent.location;
}

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

If I comment out everything in wallflower.js, the number of compartments used during the test seems to stay below 10.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And if I change inIframe() to return true or false, compartment count stays below 10.

So something about window.llocation or window.parent.location is the leak?

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Apologies for bugspam, but I played around with this some more.

When I change inIframe() to 
  return "window.location" != window.parent.location;
The page gets hundreds/thousands of compartments.

If I change it to
  return window.location != "window.parent.location";
Compartment count stays around 10.



And if I replace wallflower.js with
(function killIframes() {
  window.parent.location;
})()
Compartment count explodes.

So something about window.parent.location is leaky?

Comment 11

[Kris Maglione [:kmag]]

What about just touching window.parent? Or some other arbitrary property of window.parent? I doubt the .location property has anything to do with it unless maybe it throws.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(function killIframes() {
  window.parent;
})()

This has a huge compartment count.
I'm testing this with Firefox 14, maybe this is less bad on 15+?

Comment 13

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Wes Kocher (:KWierso) from comment #12)
...
> I'm testing this with Firefox 14, maybe this is less bad on 15+?

Please test on 15 - would love to know if there is a difference.

Comment 14

[Kris Maglione [:kmag]]

I tested Wallflower on 14 and 15 with the same results.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

No difference, other than 15 being more specific about the compartment:
http://test262.ecmascript.org/#, resource://jid0-6fmldqutys5lzsa5qun5xsyggns-at-jetpack/api-utils/lib/loader.js -> resource://jid0-6fmldqutys5lzsa5qun5xsyggns-at-jetpack/api-utils/lib/sandbox.js [810]

Comment 16

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I'd say more specificity is a win?

This is turning into an awesome bug, especially wrt the behaviour detailed in comment 10.

Comment 17

[Dietrich Ayala (:dietrich)]

I found this bug via the Jetpack weekly meeting notes. The cc'd address wasn't my bugzilla account :)

The code in question is in a page-mod, so you should be able to do any of the test-cases that Wes has come up with in a regular content page to reproduce, right?

Comment 18

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Dietrich Ayala (:dietrich) from comment #17)
> I found this bug via the Jetpack weekly meeting notes. The cc'd address
> wasn't my bugzilla account :)

Sorry!

> The code in question is in a page-mod, so you should be able to do any of
> the test-cases that Wes has come up with in a regular content page to
> reproduce, right?

Maybe - might there be additional complications with this code because it is in a sandbox?

Comment 19

[Dietrich Ayala (:dietrich)]

Yes, sandbox complications. Here are some fun testcases from different ways of detecting whether that pagemod is running in an iframe, when i know that in fact it is because i'm dumping the URL.

* window.parent - any access of it results in: "TypeError: cyclic object value"

* window.top == window is... true! even when in an iframe! wtf!

* window.frameElement throws

* window.top.location.href throws

* self.frameElement doesn't throw, but it's not correct either

I'm sure there's more.

Right now, the compartment-frenzy approach is the only thing I can get to work. Open to suggestions :)

Comment 20

[Jeff Griffiths (:canuckistani) (:⚡︎)]

var isFrame = (unsafeWindow.self !== unsafeWindow.top)

^^ This seems to work reliably in content scripts.

Comment 21

[Dietrich Ayala (:dietrich)]

Does that have the compartment problem?

Comment 22

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I did some testing, it does not seem to. I do not *think* that the use of unsafeWindow is a problem there, no data from unsafeWindow goes anywhere.

Comment 23

[Dietrich Ayala (:dietrich)]

new version: https://addons.mozilla.org/en-US/firefox/addon/wallflower-1/versions/

still have to fix this bug though. we can't tell people that you can use pagemods to write addons if the web platform in that context is a footgun.

Comment 24

[Alexandre Poirot [:ochameau]]

This leak is related to bug 764831.

Comment 25

[Alexandre Poirot [:ochameau]]

dietrich, Based on comment 19, I was able to reproduce window.top==window and submitted a fix in bug 784431. I'm wondering if you are able to reproduce all these issues with master version of the SDK?
I've added some tests which tend to say that everything works fine (except top):
  https://github.com/mozilla/addon-sdk/pull/529/files#L1R478

Otherwise, bug 684047 will avoid creating content script for iframes and ease the development of tons of jetpack addons (and save a bunch of memory)!

Comment 26

[Kris Maglione [:kmag]]

window.top == window was, I think, a wrapper bug that's fixed in Aurora. I saw something similar the other week, at any rate, and couldn't reproduce it in a more recent build.

Comment 27

[Alexandre Poirot [:ochameau]]

Should be fixed by bug 786976, using upcoming SDK 1.11 and Firefox 17+. 
No beta available yet as we are still on 1.10 train. 1.11b1 should be due to 18th September

Unfortunately, it is really hard to fix this leak in our JS proxies code.

Comment 28

[Alexandre Poirot [:ochameau]]

Considering this as being fixed as we are no longer using JS proxies since Firefox 17. But, you may have to repack your addon with last SDK.