Last Comment Bug 837729 - when Facebook Messenger is enabled, we create textruns for huge chunks of JS source code
: when Facebook Messenger is enabled, we create textruns for huge chunks of JS ...
Status: RESOLVED FIXED
[MemShrink][QA-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 21
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
Depends on:
Blocks: 805246
  Show dependency treegraph
 
Reported: 2013-02-04 09:12 PST by Jonathan Kew (:jfkthame)
Modified: 2013-03-28 05:38 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
fixed
fixed


Attachments
investigative patch, report creation of unusually-long textruns (1.30 KB, patch)
2013-02-04 09:12 PST, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
don't create text runs for JS source code loaded in a hidden-window iframe (1.24 KB, patch)
2013-02-04 11:24 PST, Jonathan Kew (:jfkthame)
gavin.sharp: review+
markh: feedback+
Details | Diff | Splinter Review
followup - discard JS source text from the DOM once it has been extracted (1.44 KB, patch)
2013-02-04 12:00 PST, Jonathan Kew (:jfkthame)
markh: review+
Details | Diff | Splinter Review
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it. (2.32 KB, patch)
2013-02-05 11:46 PST, Jonathan Kew (:jfkthame)
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it. (2.36 KB, patch)
2013-02-05 11:53 PST, Jonathan Kew (:jfkthame)
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2013-02-04 09:12:29 PST
Created attachment 709751 [details] [diff] [review]
investigative patch, report creation of unusually-long textruns

When Facebook Messenger is enabled, we seem to be creating gfxTextRuns (as if we're trying to do text measurement or rendering) for large pieces of JavaScript source code. This seems quite pointless; it's burning up CPU cycles and considerable amounts of memory to "shape" text runs that AFAICT will never be needed. We don't display this source code as visible text anywhere, so why do we build gfxTextRuns for it?

The attached investigative patch reports any time we call gfxFontGroup::InitTextRun to initialize a run with over 15000 characters - which is pretty rare in normal web browsing.

With Messenger enabled, I consistently hit this during the initial launch of the browser, before even logging in to Facebook or restoring the previous session's tabs. The text run being created is clearly some JS source (perhaps from Facebook's social-provider code?):

### gfxFontGroup::InitTextRun handling a very long text run (19171 chars)!
----------
[] = [];if (typeof window === "undefined") { self = {};
__DEV__ = 0; self["__DEV__"] = 0;window = {};window.setTimeout = function (cb, time) { return setTimeout(cb, time); };window.setInterval = function (cb, time) { return setInterval(cb, time); };window.clearTimeout = function (id) { return clearTimeout(id); };window.clearInterval = function (id) { return clearInterval(id); };window.location = {};window.location.hash = "";window.location.host = "www.facebook.com";window.location.hostname = "ww (...etc...)
----------

Opening the sidebar and logging in to Facebook triggers several more instances of similar behavior:

### gfxFontGroup::InitTextRun handling a very long text run (57381 chars)!
----------
[] = [];if (typeof window === "undefined") { self = {};
__DEV__ = 0; self["__DEV__"] = 0;window = {};window.setTimeout = function (cb, time) { return setTimeout(cb, time); };window.setInterval = function (cb, time) { return setInterval(cb, time); };window.clearTimeout = function (id) { return clearTimeout(id); };window.clearInterval = function (id) { return clearInterval(id); };window.location = {};window.location.hash = "";window.location.host = "www.facebook.com";window.location.hostname = "ww (...etc...)
----------

### gfxFontGroup::InitTextRun handling a very long text run (153210 chars)!
----------
__d("AsyncRequest",["Event","Arbiter","AsyncResponse","Bootloader","CSS","Env","HTTPErrors","JSCC","Parent","Run","ServerJS","URI","UserAgent","XHR","bind","copyProperties","emptyFunction","evalGlobal","ge","goURI","isEmpty","tx"],function(a,b,c,d,e,f){var g=b('Event'),h=b('Arbiter'),i=b('AsyncResponse'),j=b('Bootloader'),k=b('CSS'),l=b('Env'),m=b('HTTPErrors'),n=b('JSCC'),o=b('Parent'),p=b('Run'),q=b('ServerJS'),r=b('URI'),s=b('UserAgent'),t=b('XHR'),u=b('bind'),v=b('copyProperties'),w=b('empty (...etc...)
----------

### gfxFontGroup::InitTextRun handling a very long text run (366844 chars)!
----------
__d("DataSource",["array-extensions","ArbiterMixin","AsyncRequest","TypeaheadUtil","copyProperties","createArrayFrom","createObjectFrom","emptyFunction"],function(a,b,c,d,e,f){b('array-extensions');var g=b('ArbiterMixin'),h=b('AsyncRequest'),i=b('TypeaheadUtil'),j=b('copyProperties'),k=b('createArrayFrom'),l=b('createObjectFrom'),m=b('emptyFunction');function n(o){this._maxResults=o.maxResults||10;this.token=o.token;this.queryData=o.queryData||{};this.queryEndpoint=o.queryEndpoint||'';this.boots (...etc...)
----------

### gfxFontGroup::InitTextRun handling a very long text run (19828 chars)!
----------
__d("AsyncRequest",["Event","Arbiter","AsyncResponse","Bootloader","CSS","Env","HTTPErrors","JSCC","Parent","Run","ServerJS","URI","UserAgent","XHR","bind","copyProperties","emptyFunction","evalGlobal","ge","goURI","isEmpty","tx"],function(a,b,c,d,e,f){var g=b('Event'),h=b('Arbiter'),i=b('AsyncResponse'),j=b('Bootloader'),k=b('CSS'),l=b('Env'),m=b('HTTPErrors'),n=b('JSCC'),o=b('Parent'),p=b('Run'),q=b('ServerJS'),r=b('URI'),s=b('UserAgent'),t=b('XHR'),u=b('bind'),v=b('copyProperties'),w=b('empty (...etc...)
----------

### gfxFontGroup::InitTextRun handling a very long text run (19087 chars)!
----------
__d("Dialog",["array-extensions","Event","Animation","Arbiter","AsyncRequest","Bootloader","Button","ContextualThing","CSS","DOM","Focus","Form","HTML","Keys","Locale","Parent","Run","Style","URI","UserAgent","Vector","bind","copyProperties","createArrayFrom","emptyFunction","getObjectValues","getOverlayZIndex","tx"],function(a,b,c,d,e,f){b('array-extensions');var g=b('Event'),h=b('Animation'),i=b('Arbiter'),j=b('AsyncRequest'),k=b('Bootloader'),l=b('Button'),m=b('ContextualThing'),n=b('CSS'),o= (...etc...)
----------

One of those runs apparently contains 360K of minified JS source... creating a textRun for this will take around 1.5MB of RAM just for the run itself, in addition to various other overhead.

We need to figure out why we're building these textRuns, and stop doing it; we're wasting millions of CPU cycles and megabytes of space here.
Comment 1 Jonathan Kew (:jfkthame) 2013-02-04 09:46:31 PST
The JS source code appearing in these runs doesn't seem to live anywhere in mozilla-central, AFAICT, so I suspect it's source that comes from Facebook. But why are textruns being created for it? I guess that could either be a problem with the SocialAPI code on our side, or some kind of poor coding on theirs. But either way, it's a bad thing.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-04 10:54:55 PST
The "FrameWorker" component (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/FrameWorker.jsm) uses an iframe in the hidden window to load the JS for the provider's "Worker". We need the iframe because we need some APIs that are associated with windows (XHR, WebSocket, etc.), and we need it to have the provider's origin, and so we also use it to load the JS (and then just take the document's textContent and run it in a sandbox). This is almost certainly where these textruns are coming from.

This iframe is always hidden, though, and so ideally we wouldn't waste memory on anything presentation-related. Is there any way we can avoid this overhead, e.g. by styling the document somehow?
Comment 3 Jonathan Kew (:jfkthame) 2013-02-04 11:24:24 PST
Created attachment 709807 [details] [diff] [review]
don't create text runs for JS source code loaded in a hidden-window iframe

Aha, that's where they come from. Setting display:none on the iframe that's created will save us doing a ton of pointless work, then.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-04 11:27:26 PST
Comment on attachment 709807 [details] [diff] [review]
don't create text runs for JS source code loaded in a hidden-window iframe

Looks good to me, though I haven't tested it. Flagging MarkH in case he can think of anything this might negatively affect. I suspect we'll want to uplift this to Aurora at least.
Comment 5 Jonathan Kew (:jfkthame) 2013-02-04 11:31:00 PST
(Though I wonder whether inserting the JS source text into the DOM in the first place isn't perhaps an inefficient way to do this?)
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-04 11:33:08 PST
We need to load something from the provider's domain, I think. That could in theory be a lighter-weight document, but with the iframe display:none I suspect the difference would be negligible. We could kill the text in the DOM after we've extracted it, though.
Comment 7 Jonathan Kew (:jfkthame) 2013-02-04 12:00:44 PST
Created attachment 709822 [details] [diff] [review]
followup - discard JS source text from the DOM once it has been extracted

Seems like this should reduce ongoing memory footprint a little further, then.
Comment 8 Shane Caraveo (:mixedpuppy) 2013-02-04 14:40:27 PST
drive by...both these patches pass tests for me locally.
Comment 9 Mark Hammond [:markh] 2013-02-04 15:22:04 PST
Comment on attachment 709822 [details] [diff] [review]
followup - discard JS source text from the DOM once it has been extracted

awesome!
Comment 10 Jonathan Kew (:jfkthame) 2013-02-05 01:23:31 PST
Folded these two (one-line) patches into a single changeset on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdf53493d31
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-02-05 07:32:21 PST
https://hg.mozilla.org/mozilla-central/rev/fbdf53493d31
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2013-02-05 10:54:40 PST
These patches seem very simple and low risk. Johnathan, can you nominate for aurora? I'm not sure if it is too late to make the last beta for 19.
Comment 13 Jonathan Kew (:jfkthame) 2013-02-05 11:46:13 PST
Created attachment 710323 [details] [diff] [review]
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it.

Combined patch, as landed on central, ready for aurora uplift if approved.
Comment 14 Jonathan Kew (:jfkthame) 2013-02-05 11:49:49 PST
Comment on attachment 710323 [details] [diff] [review]
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): SocialAPI

User impact if declined: enabling Social features is less performant than it should be: we perform unnecessary layout on huge pieces of JS source code, wasting cycles and churning megabytes of RAM for no purpose

Testing completed (on m-c, etc.): tested locally, now in m-c

Risk to taking this patch (and alternatives if risky): trivial patch with minimal risk

String or UUID changes made by this patch: none
Comment 15 Jonathan Kew (:jfkthame) 2013-02-05 11:53:04 PST
Created attachment 710326 [details] [diff] [review]
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it.

As above but for mozilla-beta; the actual code change is identical, but patch didn't apply cleanly because of error-handling changes in the context, so this is the rebased version for beta.
Comment 16 Jonathan Kew (:jfkthame) 2013-02-05 11:55:20 PST
Comment on attachment 710326 [details] [diff] [review]
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it.

[Approval Request Comment]

As per comment 14.
Comment 17 Alex Keybl [:akeybl] 2013-02-05 13:02:54 PST
Comment on attachment 710326 [details] [diff] [review]
don't create text-runs for JS source code loaded in a hidden-window iframe, and remove source text from the DOM as soon as we've extracted it.

Gavin agrees that this isn't critical enough to fix in FF19, given how close we are to release. Approving for Aurora, however.
Comment 18 Jonathan Kew (:jfkthame) 2013-02-05 13:24:13 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/5385979f826d
Comment 19 Mihai Morar, (:MihaiMorar) 2013-03-28 02:02:46 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #18)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/5385979f826d

How can QA verify this fix?
Comment 20 Jonathan Kew (:jfkthame) 2013-03-28 02:57:26 PDT
I'm not sure you can, really. The goal here is to avoid some unnecessary work during startup and especially when opening the Facebook sidebar. So there'll be a perf improvement, but it will be impossible to detect that reliably among the overall operation of opening the Facebook sidebar.

You -might- be able to detect the reduction in memory footprint (at least a couple of MB) in about:memory, but again, I suspect it'll be hard to spot that among the "noise" of other variations from one run to another.
Comment 21 Mihai Morar, (:MihaiMorar) 2013-03-28 03:06:31 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #20)

In this case can you confirm this is fixed? So we can move it from Resolved Fixed to Verified.
Comment 22 Jonathan Kew (:jfkthame) 2013-03-28 04:06:53 PDT
I can confirm that the code is present in mozilla-central and aurora repositories. Also, in a local debug build that includes the original diagnostic patch from comment #0, the huge textruns are no longer reported. So I think that confirms it is working as expected.

Don't think there's much else you can do here - you can't directly "confirm" it with a standard release build because the instrumentation that reveals the huge textruns is not present.
Comment 23 Mihai Morar, (:MihaiMorar) 2013-03-28 05:38:13 PDT
Based on Comment 20 and Comment 22 marking as [qa-]

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