With child process sandboxing, the current approach of reading from a file (ua-update.json) will not work anymore.

(In reply to Carsten Book [:Tomcat] from comment #1) > https://hg.mozilla.org/mozilla-central/rev/dbc405bc63b5 Wrong bug number.

Tested on Otoro and b2g-desktop emulator. Still needs to run through mochitest / try server.

Comment on attachment 820804 [details] [diff] [review] uaipac_bug927633.patch Review of attachment 820804 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/SiteSpecificUserAgent.js @@ +20,5 @@ > > SiteSpecificUserAgent.prototype = { > getUserAgentForURIAndWindow: function ssua_getUserAgentForURIAndWindow(aURI, aWindow) { > + var data = { uri: aURI }; > + return cpmm.sendSyncMessage("Useragent:GetOverride", data)[0] || HTTP_PROTO_HANDLER.userAgent; how long does it take to do this sync call? We really try to not do sync calls in general, that may slow down loads a lot.

Fixes mochitests by removing check for only XPCOM wrapped URLs. @Fabrice - Sync calls are probably pretty expensive, but previously it was implemented as a sync call. If we change it to an async call, don't we have to also change Navigator.cpp to be async as well? (http://mxr.mozilla.org/mozilla-aurora/source/dom/base/Navigator.cpp#306).

Comment on attachment 821414 [details] [diff] [review] uaipc.patch Review of attachment 821414 [details] [diff] [review]: ----------------------------------------------------------------- Drive-by comments: The sync call only happens when JS specifically uses navigator.userAgent, which is not common in my experience. Also, we can use a simple cache in SiteSpecificUserAgent.js to avoid unnecessary sync calls. See the Map-based cache used in UserAgentOverrides.jsm. ::: dom/base/SiteSpecificUserAgent.js @@ +7,4 @@ > const Ci = Components.interfaces; > > Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > Cu.import("resource://gre/modules/UserAgentOverrides.jsm"); Lazy import UserAgentOverrides.jsm only for parent process @@ +20,5 @@ > > SiteSpecificUserAgent.prototype = { > getUserAgentForURIAndWindow: function ssua_getUserAgentForURIAndWindow(aURI, aWindow) { > + var data = { uri: aURI }; > + return cpmm.sendSyncMessage("Useragent:GetOverride", data)[0] || HTTP_PROTO_HANDLER.userAgent; I think you want to check if we are actually in the child or not. Use old code if we are in the parent. ::: netwerk/protocol/http/UserAgentOverrides.jsm @@ +22,5 @@ > +XPCOMUtils.defineLazyServiceGetter(this, "ppmm", > + "@mozilla.org/parentprocessmessagemanager;1", > + "nsIMessageListenerManager"); // Might have to make this broadcast? > + > +const OVERRIDE_MESSAGE = "Useragent:GetOverride"; Group this with other consts @@ +40,5 @@ > > gPrefBranch = Services.prefs.getBranch("general.useragent.override."); > gPrefBranch.addObserver("", buildOverrides, false); > > + ppmm.addMessageListener(OVERRIDE_MESSAGE, this); Is PPMM available on non-b2g platforms? @@ +69,5 @@ > > getOverrideForURI: function uao_getOverrideForURI(aURI) { > if (!gInitialized || > (!gOverrides.size && !gUpdatedOverrides) || > + !(aURI.asciiHost)) { Can we only forward URI.asciiHost through IPC since we're only using asciiHost? @@ +124,5 @@ > + case OVERRIDE_MESSAGE: > + var uri = aMessage.data.uri; > + return this.getOverrideForURI(uri); > + default: > + dump("Wrong Message in UserAgentOverride: " + name); Throw an error instead of dump(). @@ +163,5 @@ > function HTTP_on_modify_request(aSubject, aTopic, aData) { > let channel = aSubject.QueryInterface(Ci.nsIHttpChannel); > > for (let callback of gOverrideFunctions) { > + var modifiedUA = callback(channel, DEFAULT_UA); Use "let" instead of "var", and in other places.

Comment on attachment 821414 [details] [diff] [review] uaipc.patch Review of attachment 821414 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review until you address previous comments.

Successful try server: https://tbpl.mozilla.org/?tree=Try&rev=2e0a207ffae7 > The sync call only happens when JS specifically uses navigator.userAgent, > which is not common in my experience. Also, we can use a simple cache in > SiteSpecificUserAgent.js to avoid unnecessary sync calls. See the Map-based > cache used in UserAgentOverrides.jsm. After discussing with Fabrice, we don't want to use a cache in SiteSpecificUserAgent. If we did, the parent would have to send a call to invalidate all children when a http-modify-request comes into the parent. Also, as Fabrice stated, SiteSpecificUserAgent is only used when navigator.useragent is accessed. I made a web page that just wrote "hello world", and this call path was not taken until navigator.userAgent was accessed. > Can we only forward URI.asciiHost through IPC since we're only using asciiHost? I thought about this, but then we'd have to modify all the callers to fetch asciiHost, which seems like a bigger change for not a lot of win.

(In reply to Jim Chen [:jchen :nchen] from comment #6) > The sync call only happens when JS specifically uses navigator.userAgent, > which is not common in my experience. What is this experience based on? Did you examine a somewhat representative set of scripts found on the web? I would actually expect it to be used pretty often in scripts and libraries doing browser detection a.k.a. UA sniffing.

(In reply to Dão Gottwald [:dao] from comment #9) > (In reply to Jim Chen [:jchen :nchen] from comment #6) > > The sync call only happens when JS specifically uses navigator.userAgent, > > which is not common in my experience. > > What is this experience based on? Did you examine a somewhat representative > set of scripts found on the web? > > I would actually expect it to be used pretty often in scripts and libraries > doing browser detection a.k.a. UA sniffing. It was from a quick look at some popular JS libraries like jQuery, MooTools, backbone, etc. By "common" I meant using navigator.userAgent more than once or twice per page load. Maybe "frequent" is a better word here. IMO once or twice per page load is reasonable for a sync call (but that's up for debate). So while navigator.userAgent is used for UA sniffing in many of the libraries I looked at, they don't seem to use navigator.userAgent excessively, which would make sync calls bad here.

Mason, can you do some simple measurements of how long these sync calls take while browsing common sites?

Here are some simple measurements. I wrote this code on a webpage: var start = new Date().getMilliseconds(); var userAgent = navigator.userAgent; var end = new Date().getMilliseconds(); var timing = end - start; document.write("User agent: " + userAgent + " timing= " + timing); Measurements indicate the number of milliseconds to just read the user agent. This was tested on an otoro device with 2 revisions: 1 with an unmodified mozilla-aurora gecko branch, and another using the proposed patch attached to this bug. Average taken over 10 runs by pushing refresh on the browser. First number is biased due to cold start. Timings sync call: 11 ms average. Raw numbers in ms (32, 8, 15, 9, 4, 8, 8, 7, 10, 9). Timings unmodified: 5.8 ms average. Raw numbers in ms (17, 2, 8, 5, 3, 8, 7, 5, 2, 1). Roughly, using a sync call is 2x as expensive a direct call if in the same process. Please let me know if you need anything else.

Redid with 1000 iterations. Also redid numbers to use new Date() only w/o getMilliseconds. getMilliseconds had the wrong semantics, which gets the number of milliseconds for the current time, (0-999), not the date in milliseconds. For a single iteration: Timings sync call: 9.3 ms Timings unmodified: 5.7 ms So 63% slower using a sync call. For 1000 calls to navigator.userAgent (averaged over 10 runs): Timings sync call: 2747.5 ms Timings unmodified: 259.9 ms Roughly ~10x slowdown. If navigator.userAgent isn't accessed very often, a ~3.6 ms penalty may be worth the 1.5 mb memory savings. If this isn't the case, then we can try to build the cache in SiteSpecificUserAgent.js.

(In reply to Jim Chen [:jchen :nchen] from comment #10) > It was from a quick look at some popular JS libraries like jQuery, MooTools, > backbone, etc. > > By "common" I meant using navigator.userAgent more than once or twice per > page load. Maybe "frequent" is a better word here. IMO once or twice per > page load is reasonable for a sync call (but that's up for debate). So while > navigator.userAgent is used for UA sniffing in many of the libraries I > looked at, they don't seem to use navigator.userAgent excessively, which > would make sync calls bad here. Random data point, just spotted on planet.mozilla.org: http://www.otsukare.info/2013/10/25/meta-refresh-http (look for "One of the baidu script has a pretty interesting piece of code")

~10x slowdown is not acceptable unfortunately... We'll need to bite the bullet here.

Implemented a cache on SiteSpecificUserAgent with an invalidation while observing to the same preferences as UserAgentUpdates.jsm. A nice side effect of the cache is that we actually speed fetching a user agent up a little bit! Benchmarks to fetch 1000 navigator.userAgents, averaged over 10 runs: Original implementation: 259.9 ms With Patch: 195.5 ms! A savings of 64.4ms! A few design questions though: 1) There's no uninit in SiteSpecificUserAgent, so we never clean up the observer. Should we add an uninit() call in all places we find UserAgentOverrides.uninit called? Or setup an IPC call from UserAgentOverrides.uninit() to clean up SiteSpecificUserAgent? 2) Should SiteSpecificUserAgent invalidate caches based on Preference observer as implemented, or should we have an IPC call from UserAgentOverrides to invalidate SiteSpecificUserAgent? I went with a Preference observer implementation because the Preference services was built to have observers listen on when the user agent changes, whereas we'd have to build the back/forth communication between UserAgentOverrides and SiteSpecificUserAgent.

Comment on attachment 823788 [details] [diff] [review] uaipc.patch Review of attachment 823788 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Looks good with just a couple comments. Now you can get rid of the block here, http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/UserAgentUpdates.jsm?rev=922eb17b8a44#121 Also, can you post new numbers for your benchmark? ::: dom/base/SiteSpecificUserAgent.js @@ +10,2 @@ > > +const PREF_OVERRIDE = "general.useragent.override."; You want to observe "general.useragent.updates." as well, for when updates occur. ::: netwerk/protocol/http/UserAgentOverrides.jsm @@ +40,5 @@ > > gPrefBranch = Services.prefs.getBranch("general.useragent.override."); > gPrefBranch.addObserver("", buildOverrides, false); > > + ppmm.addMessageListener(OVERRIDE_MESSAGE, this); Have you verified that this call will work on non-B2G platforms?

Carrying over the r+ from :nchen in Comment 17. Fixes the comments, deleted the block requested and observe "general.useragent.updates."preference. >> Have you verified that this call will work on non-B2G platforms? Previously passed try server, so it should work on non-b2g platforms. I haven't encountered any issues running mochitest locally. Here is another try server just in case: https://tbpl.mozilla.org/?tree=Try&rev=f4752ead15c5

Performance numbers for the benchmark (read navigator.userAgent 1000 times in a loop): Timings w/ Patch: Average: 205.2 ms Raw data (time in ms): 190, 188, 235, 314, 181, 177, 195, 210, 168, 194. Timings w/o Patch: Average: 264.1 Raw Data (time in ms): 263, 275, 315, 236, 239, 257, 266, 253, 278, 259

(In reply to Mason Chang [:mchang] from comment #18) > Created attachment 824314 [details] [diff] [review] > uaipc.patch > > Carrying over the r+ from :nchen in Comment 17. That was a feedback+, which usually means that the reviewer wants to take another look at the updated patch...

Comment on attachment 824314 [details] [diff] [review] uaipc.patch Review of attachment 824314 [details] [diff] [review]: ----------------------------------------------------------------- Mason, sorry I missed your earlier comments. (In reply to Mason Chang [:mchang] from comment #16) > > 1) There's no uninit in SiteSpecificUserAgent, so we never clean up the > observer. Should we add an uninit() call in all places we find > UserAgentOverrides.uninit called? Or setup an IPC call from > UserAgentOverrides.uninit() to clean up SiteSpecificUserAgent? Yes, you should unregister the observer. I think you can register for the "xpcom-shutdown" event and unregister both the "xpcom-shutdown" event and the pref change event from there. For an example, see http://mxr.mozilla.org/mozilla-central/source/dom/base/ConsoleAPIStorage.jsm?rev=f4ec1953f7e5#45 > 2) Should SiteSpecificUserAgent invalidate caches based on Preference > observer as implemented, or should we have an IPC call from > UserAgentOverrides to invalidate SiteSpecificUserAgent? I went with a > Preference observer implementation because the Preference services was built > to have observers listen on when the user agent changes, whereas we'd have > to build the back/forth communication between UserAgentOverrides and > SiteSpecificUserAgent. I think observing prefs is fine. (In reply to Dão Gottwald [:dao] from comment #20) > > That was a feedback+, which usually means that the reviewer wants to take > another look at the updated patch... Yes. Please request review again after updating a feedback+ patch.

Added observer on xpcom-shutdown to clean everything up. >> That was a feedback+, which usually means that the reviewer wants to take another look at the updated patch... Woops sorry! Didn't know that, thanks for the heads up.

Comment on attachment 824762 [details] [diff] [review] uaipc.patch Review of attachment 824762 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Looks good to me. One extra note is you may want to keep the cache to a certain size (see http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/UserAgentOverrides.jsm?rev=60185e9edfd4#93)

Carrying over the r+, added change to clear out the cache if greater than 250 entries. Hopefully last try server build: https://tbpl.mozilla.org/?tree=Try&rev=4158b7a1798e

Blocks a blocker, and we need this to get memory usage in 1.2 down towards 1.1 levels. See https://bugzilla.mozilla.org/show_bug.cgi?id=921659#c40

Koi+ for perf issue.

Comment on attachment 824880 [details] [diff] [review] uaipc.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 921659 User impact if declined: b2g preallocated app will consume 1.5 mb of memory. Testing completed (on m-c, etc.): Landed on m-c Risk to taking this patch (and alternatives if risky): String or IDL/UUID changes made by this patch: N/A

What's the point of taking this on Aurora?