Closed Bug 837729 Opened 12 years ago Closed 12 years ago

when Facebook Messenger is enabled, we create textruns for huge chunks of JS source code

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(firefox18 wontfix, firefox19 wontfix, firefox20 fixed, firefox21 fixed)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- fixed
firefox21 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

(Whiteboard: [MemShrink][QA-])

Attachments

(5 files)

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.
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.
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?
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.
Attachment #709807 - Flags: review?(gavin.sharp)
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.
Attachment #709807 - Flags: review?(gavin.sharp)
Attachment #709807 - Flags: review+
Attachment #709807 - Flags: feedback?(mhammond)
(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?)
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.
Seems like this should reduce ongoing memory footprint a little further, then.
Attachment #709822 - Flags: review?(gavin.sharp)
Attachment #709822 - Flags: review?(gavin.sharp) → review?(mhammond)
drive by...both these patches pass tests for me locally.
Attachment #709807 - Flags: feedback?(mhammond) → feedback+
Comment on attachment 709822 [details] [diff] [review] followup - discard JS source text from the DOM once it has been extracted awesome!
Attachment #709822 - Flags: review?(mhammond) → review+
Assignee: nobody → jfkthame
Blocks: 805246
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Folded these two (one-line) patches into a single changeset on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdf53493d31
Target Milestone: --- → Firefox 21
Whiteboard: [MemShrink]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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 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
Attachment #710323 - Flags: approval-mozilla-aurora?
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 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.
Attachment #710326 - Flags: approval-mozilla-beta?
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.
Attachment #710326 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #710323 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Jonathan Kew (:jfkthame) from comment #18) > https://hg.mozilla.org/releases/mozilla-aurora/rev/5385979f826d How can QA verify this fix?
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.
(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.
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.
Based on Comment 20 and Comment 22 marking as [qa-]
Whiteboard: [MemShrink] → [MemShrink][QA-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: