891968 - We have hundreds of copies of the UA string override on B2G

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Andrew McCreight [:mccr8]]

I'm not sure if this is the right component, and I'm not sure how big of a deal this is, but it looks like the user agent override is holding alive a few hundreds copies of the UA string 'Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1'.  It would be good if we could avoid this somehow.

Here is a list of paths keeping alive one particular copy of this string. The other ones look similar, except with a different URL in the final step.  Let me know if you have trouble understanding this.

via resource://gre/modules/UserAgentOverrides.jsm :
0x4481a5a0 [FakeBackstagePass <no private>]
    --[UserAgentOverrides]-> 0x48e7dd60 [Object <no private>]
    --[getOverrideForURI]-> 0x48ebdd60 [Function uao_getOverrideForURI]
    --[fun_callscope]-> 0x44820820 [Call <no private>]
    --[gOverrides]-> 0x4a36f3a0 [Object <no private>]
    --[deadline.com]-> 0x4a343c30 [string Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1]

via XPCWrappedNative::mFlatJSObject :
0x48e46040 [ChromeWindow 48c77160]
    --[UserAgentOverrides]-> 0x48e4b5e0 [Proxy <no private>]
    --[private]-> 0x48e7dd60 [Object <no private>]
    --[getOverrideForURI]-> 0x48ebdd60 [Function uao_getOverrideForURI]
    --[fun_callscope]-> 0x44820820 [Call <no private>]
    --[gOverrides]-> 0x4a36f3a0 [Object <no private>]
    --[deadline.com]-> 0x4a343c30 [string Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1]

via nsXPCWrappedJS[nsIObserver,0x4a040400:0x47de6100].mJSObj :
0x48ebdcc0 [Function buildOverrides]
    --[fun_callscope]-> 0x44820820 [Call <no private>]
    --[gOverrides]-> 0x4a36f3a0 [Object <no private>]
    --[deadline.com]-> 0x4a343c30 [string Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1]

via nsXPCWrappedJS[nsIObserver,0x4a040480:0x47de6140].mJSObj :
0x48ebdce0 [Function HTTP_on_modify_request]
    --[fun_callscope]-> 0x44820820 [Call <no private>]
    --[gOverrides]-> 0x4a36f3a0 [Object <no private>]
    --[deadline.com]-> 0x4a343c30 [string Mozilla/5.0 (Android; Mobile; rv:18.1) Gecko/18.1 Firefox/18.1]

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

With http://mxr.mozilla.org/gaia/source/build/ua-override-prefs.js?raw=1 applied on desktop, about:memory gave me the following numbers.

before:

73,504 B (00.04%) -- compartment([System Principal], resource://gre/modules/UserAgentOverrides.jsm)
├──49,488 B (00.03%) -- gc-heap
│  ├──20,360 B (00.01%) -- shapes
│  │  ├──11,320 B (00.01%) ── tree/global-parented
│  │  └───9,040 B (00.01%) ── dict
│  ├──16,448 B (00.01%) ── objects/function
│  └──12,680 B (00.01%) ── sundries
├──14,800 B (00.01%) ── other-sundries
└───9,216 B (00.01%) ── shapes-extra/compartment-tables

after:

63,584 B (00.04%) -- compartment([System Principal], resource://gre/modules/UserAgentOverrides.jsm)
├──43,584 B (00.03%) -- gc-heap
│  ├──16,512 B (00.01%) ── objects/function
│  ├──15,632 B (00.01%) ── sundries
│  └──11,440 B (00.01%) ── shapes/tree/global-parented
├──10,784 B (00.01%) ── other-sundries
└───9,216 B (00.01%) ── shapes-extra/compartment-tables

Is this the difference you'd expect?

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch v2

cleaned it up a bit. new "after" measurement:

63,320 B (00.03%) -- compartment([System Principal], resource://gre/modules/UserAgentOverrides.jsm)
├──43,416 B (00.02%) -- gc-heap
│  ├──16,512 B (00.01%) ── objects/function
│  ├──15,504 B (00.01%) ── sundries
│  └──11,400 B (00.01%) ── shapes/tree/global-parented
├──10,688 B (00.01%) ── other-sundries
└───9,216 B (00.01%) ── shapes-extra/compartment-tables

Comment 3

[Dão Gottwald [:dao]]

My patches apply on top of bug 896382's patch, which I just landed on inbound.

Comment 4

[Andrew McCreight [:mccr8]]

From the memory report there, it looks like you are saving 10k in shapes/dict, which I think would be the result of switching from an object to a map.  So that's good, but it doesn't involve the strings per se.  I don't actually see strings in that bit of the report at all.

If you get me a GC heap dump (using this https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump ) then I can see if there are less strings floating around, or you can look at it yourself using this script: https://github.com/amccreight/heapgraph/blob/master/gc/stringy.py

Comment 5

[Dão Gottwald [:dao]]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> If you get me a GC heap dump (using this
> https://wiki.mozilla.org/Performance:Leak_Tools#Cycle_collector_heap_dump )

I was unable to locate the log file. No idea where it was written to. It's not in the path Firefox was running from.

Comment 6

[Andrew McCreight [:mccr8]]

It should print out the place it dumps it to in the Browser Console.  On OSX, I think it ends up in ~/Library/Caches/TemporaryItems.  On Linux it is in /tmp I think

Comment 7

[Dão Gottwald [:dao]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> It should print out the place it dumps it to in the Browser Console.

I just got "undefined" back.

> On Linux it is in /tmp I think

Ok, got it.

Comment 8

[Dão Gottwald [:dao]]

Attachment: gc-edges.3608.log.bz2

Comment 9

[Andrew McCreight [:mccr8]]

Yeah, I only see 19 strings in there that starting with "Mozilla", so that looks like an improvement, assuming what I was seeing before wasn't an artifact of the leak, which seems totally reasonable.  Thanks.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v3

more cleanup

Comment 11

[Dão Gottwald [:dao]]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Yeah, I only see 19 strings in there that starting with "Mozilla", so that
> looks like an improvement, assuming what I was seeing before wasn't an
> artifact of the leak, which seems totally reasonable.  Thanks.

I looked at the log myself and compared it with a log from an unpatched build. You definitely didn't see a leak and the patch brings UserAgentOverrides.jsm on b2g down to three strings (one for DEFAULT_UA, two in the gOverrides map).

Comment 12

[u408661]

Comment on attachment 780671 [details] [diff] [review]
patch v3

Review of attachment 780671 [details] [diff] [review]:
-----------------------------------------------------------------

This all seems reasonable to me, but given my lack of status as a necko peer, I'm going to hand off to Jason for the official review.

Jason - this touches code from the other user agent override patch I looked at earlier this week, so don't feel like you have to go over this with a fine-toothed comb or anything. I'm just following protocol :)

Comment 13

[Jason Duell]

Comment on attachment 780671 [details] [diff] [review]
patch v3

Review of attachment 780671 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine especially since nick thinks so :)

Comment 14

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/37567e5bbe9f

Comment 15

[Dão Gottwald [:dao]]

Apparently this got merged yesterday.

https://hg.mozilla.org/mozilla-central/rev/37567e5bbe9f