922342 - PermissionPromptHelper.jsm takes ~220ms on startup on a galaxy nexus

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nathan Froyd [:froydnj]]

Surely we can't be doing that much work here...

Also not sure why we're loading it in Android's browser.js, since we don't actually use it from there.  Bug 857730 apparently had similar questions, but AFAICS, they were never answered.  Seems like we ought to be lazy-loading it from ContactManager.js...

Comment 1

[Nathan Froyd [:froydnj]]

For those people who only read the comments:

http://people.mozilla.org/~bgirard/cleopatra/#report=9c53458e59ae97e51ebcc01f0bcb4d84c5de3252&filter=PermissionPrompt

Comment 2

[Nathan Froyd [:froydnj]]

Shane, do you remember why bug 857730 added PermissionPromptHelper.jsm and friends?  I saw that Chris had questions about that too, but I didn't see anything in the bug about a resolution to that question.

It looks like the expensive part of PermissionsPromptHelper.jsm is grabbing the apps service, but if we could eliminate PermissionsPromptHelper.jsm altogether, that would be even better.

Comment 3

[Shane Tully (:stully:]

Hi Nathan, ContactManager.js uses PermissionPromptHelper.jsm to show a permissions doorhanger asking the user if a webpage is allowed to use the contacts API or not.

https://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#748

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Shane Tully (:stully: from comment #3)
> Hi Nathan, ContactManager.js uses PermissionPromptHelper.jsm to show a
> permissions doorhanger asking the user if a webpage is allowed to use the
> contacts API or not.
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.
> js#748

Hm, OK.  Weird that browser.js has to be the one importing that, but I guess it makes some sense.

If we can't get rid of it, maybe we can make it less expensive.  Gregor, it looks like you wrote the code for bug 780707 where PermissionPromptHelper grabs a reference to the apps service and never uses it:

http://mxr.mozilla.org/mozilla-central/source/dom/permission/PermissionPromptHelper.jsm#46

What's the reasoning behind that piece of code?

Comment 5

[Gregor Wagner [:gwagner]]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> (In reply to Shane Tully (:stully: from comment #3)
> > Hi Nathan, ContactManager.js uses PermissionPromptHelper.jsm to show a
> > permissions doorhanger asking the user if a webpage is allowed to use the
> > contacts API or not.
> > 
> > https://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.
> > js#748
> 
> Hm, OK.  Weird that browser.js has to be the one importing that, but I guess
> it makes some sense.
> 
> If we can't get rid of it, maybe we can make it less expensive.  Gregor, it
> looks like you wrote the code for bug 780707 where PermissionPromptHelper
> grabs a reference to the apps service and never uses it:
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/permission/
> PermissionPromptHelper.jsm#46
> 
> What's the reasoning behind that piece of code?

It should be safe to remove. Maybe we need it for testing but it's my mistake if I didn't add a comment for it.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Gregor Wagner [:gwagner] from comment #5)
> (In reply to Nathan Froyd (:froydnj) from comment #4)
> > (In reply to Shane Tully (:stully: from comment #3)
> > > Hi Nathan, ContactManager.js uses PermissionPromptHelper.jsm to show a
> > > permissions doorhanger asking the user if a webpage is allowed to use the
> > > contacts API or not.
> > > 
> > > https://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.
> > > js#748
> > 
> > Hm, OK.  Weird that browser.js has to be the one importing that, but I guess
> > it makes some sense.
> > 
> > If we can't get rid of it, maybe we can make it less expensive.  Gregor, it
> > looks like you wrote the code for bug 780707 where PermissionPromptHelper
> > grabs a reference to the apps service and never uses it:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/dom/permission/
> > PermissionPromptHelper.jsm#46
> > 
> > What's the reasoning behind that piece of code?
> 
> It should be safe to remove. Maybe we need it for testing but it's my
> mistake if I didn't add a comment for it.

So testing the obvious patch leads to lots of errors during testing (e.g. breaking all the B2G emulator tests), with errors like:

11:20:23     INFO -  OpenGL version detected: 210
11:20:23     INFO -  OpenGL version detected: 210
11:20:23     INFO -  OpenGL version detected: 210
11:20:23     INFO -  1382034023706	Marionette	DEBUG	accepted connection on 127.0.0.1:34977
11:20:23     INFO -  ###################################### forms.js loaded
11:20:23     INFO -  ############################### browserElementPanning.js loaded
11:20:23     INFO -  ######################## BrowserElementChildPreload.js loaded
11:20:23     INFO -  *** UTM:SVC TimerManager:registerTimer - id: user-agent-updates-timer
11:20:23     INFO -  !! got no appInfo for system.gaiamobile.org
11:20:27     INFO -  PermissionsTable.jsm: expandPermissions: Unknown Permission: network-httpPermissionsTable.jsm: expandPermissions: Unknown Permission: network-tcpPermissionsInstaller.jsm: 'network-http' is not a valid Webapps permission name.PermissionsInstaller.jsm: 'network-tcp' is not a valid Webapps permission name.Crash reporter : Can't fetch app.reportCrashes. Exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/shell.js :: shell_reportCrash :: line 114"  data: no]Xlib:  extension "RANDR" missing on display ":0".
11:20:27     INFO -  System JS : ERROR resource://gre/modules/FileUtils.jsm:63
11:20:27     INFO -                       NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]

Gregor, do you know where these permission names are coming from?  grepping the tree didn't turn up anything obvious...

Comment 7

[Gregor Wagner [:gwagner]]

That's fine. It also happens without your patch. 
They are coming from gaia and you can find them here:
http://mxr.mozilla.org/gaia/

I filed bug 887145 for it.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: lazy-load-contacts-service.patch

does this help?

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 826151 [details] [diff] [review]
lazy-load-contacts-service.patch

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> does this help?

Yeah, this helps a lot.  ~330ms+ off cold startup eideticker on a Galaxy Nexus.  feedback+++.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 826151 [details] [diff] [review]
lazy-load-contacts-service.patch

This patch effectively disables the ContactService, since the ContactService.jsm and PermissionPromptHelper.jsm never get to call their init() methods. Those methods add listeners for messages.

We could try to find ways to lazily load the objects by listening for the messages via a delegate, kind of like we do in browser.js for other notification based objects:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#98

This uses observer notifications instead of messages, but the concept is the same.

Saving ~330ms is worth trying to find a solution though.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Move contacts JSMs to WebAppRT v0.1

This is an alternate approach to what Brad is trying.
* WebAppRT.js is already lazy loaded, only if we are opening a webapp
* Move the two JSMs to WebAppRT.js

If we launch a webapp, WebAppRT is loaded which will load (and init) the code in the two JSMs.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 8360414 [details] [diff] [review]
Move contacts JSMs to WebAppRT v0.1

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

I don't really love this. We're just pushing the problem onto someone else, and we'll hit it again if we ever expose this to the real web. For a lot of these ppmm listeners, we could use the WakeupService. It seems like it would be worth it to extend it to handle Observer notifications as well.

That said, I realize as I wrote that that these are actually jsm's, not xpcom components. No registration. No categories :( We DO have a lazy observer service for those, but not for the ppmm ones (and it sucks that everyone would have to maintain the list of messages they're registered for in two places).

So all that to say, I guess we punt on the real solution until we find it...

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3da20e4c20ec