Closed
Bug 922342
Opened 11 years ago
Closed 11 years ago
PermissionPromptHelper.jsm takes ~220ms on startup on a galaxy nexus
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: froydnj, Unassigned)
References
()
Details
(Keywords: perf)
Attachments
(2 files)
(deleted),
patch
|
mfinkle
:
review-
froydnj
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
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...
Reporter | ||
Comment 1•11 years ago
|
||
For those people who only read the comments:
http://people.mozilla.org/~bgirard/cleopatra/#report=9c53458e59ae97e51ebcc01f0bcb4d84c5de3252&filter=PermissionPrompt
Reporter | ||
Comment 2•11 years ago
|
||
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.
Flags: needinfo?(shane)
Comment 3•11 years ago
|
||
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
Flags: needinfo?(shane)
Reporter | ||
Comment 4•11 years ago
|
||
(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?
Flags: needinfo?(anygregor)
Comment 5•11 years ago
|
||
(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.
Flags: needinfo?(anygregor)
Reporter | ||
Comment 6•11 years ago
|
||
(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...
Flags: needinfo?(anygregor)
Comment 7•11 years ago
|
||
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.
Flags: needinfo?(anygregor)
Reporter | ||
Comment 9•11 years ago
|
||
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+++.
Attachment #826151 -
Flags: feedback?(nfroyd) → feedback+
Updated•11 years ago
|
Attachment #826151 -
Flags: review?(mark.finkle)
Comment 10•11 years ago
|
||
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.
Attachment #826151 -
Flags: review?(mark.finkle) → review-
Comment 11•11 years ago
|
||
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.
Attachment #8360414 -
Flags: review?(wjohnston)
Updated•11 years ago
|
Comment 12•11 years ago
|
||
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...
Attachment #8360414 -
Flags: review?(wjohnston) → review+
Comment 13•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Assignee | ||
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•