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

RESOLVED FIXED

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: till, Assigned: ochameau)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2], URL)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 648999 [details]
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.
Yeah, it sounds like the addon is leaking and causing lots of fragmentation. But the analysis-temporary thing is very odd.
Whiteboard: [MemShrink]
This uses an old version of Jetpack (1.6.1) that is known to leak in recent versions of Firefox, as I recall.
So it looks like jetpack is going off the rails here.  There are 2466 sandboxes for this one domain.  That's really broken.
Whiteboard: [MemShrink] → [MemShrink:P2]
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!
Assignee: general → nobody
Component: JavaScript Engine → General
Product: Core → Add-on SDK
Version: Trunk → unspecified
SDK 1.9 shipped today. It should be pretty leak resistant compared to 1.6.

Updated

5 years ago
Whiteboard: [MemShrink:P2] → [MemShrink:P2][triage:followup]
I warned via AMO that the add-on will be downgraded if it isn't rebuilt with a newer Jetpack.
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;
}
If I comment out everything in wallflower.js, the number of compartments used during the test seems to stay below 10.
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?
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?
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.
(function killIframes() {
  window.parent;
})()

This has a huge compartment count.
I'm testing this with Firefox 14, maybe this is less bad on 15+?
(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.
I tested Wallflower on 14 and 15 with the same results.
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]
I'd say more specificity is a win?

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

Updated

5 years ago
Assignee: nobody → poirot.alex
Priority: -- → P1
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?
(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?
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 :)
var isFrame = (unsafeWindow.self !== unsafeWindow.top)

^^ This seems to work reliably in content scripts.
Does that have the compartment problem?
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.
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.
(Assignee)

Comment 24

5 years ago
This leak is related to bug 764831.
Depends on: 764831
(Assignee)

Comment 25

5 years ago
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)!
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.
(Assignee)

Comment 27

5 years ago
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.

Updated

5 years ago
Whiteboard: [MemShrink:P2][triage:followup] → [MemShrink:P2]
(Assignee)

Comment 28

5 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.