Closed
Bug 913243
Opened 8 years ago
Closed 7 years ago
All applications are loading chrome://browser/content/ErrorPage.js
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-b2g:1.3T+, b2g-v1.3T fixed)
Tracking | Status | |
---|---|---|
b2g-v1.3T | --- | fixed |
People
(Reporter: nbp, Assigned: kk1fff)
References
Details
(Keywords: perf, Whiteboard: [c=progress p= s=2014.01.17 u=tarako])
Attachments
(5 files, 1 obsolete file)
14.07 KB,
text/plain
|
Details | |
652 bytes,
patch
|
Details | Diff | Splinter Review | |
1.71 KB,
text/x-python
|
Details | |
7.28 KB,
patch
|
kk1fff
:
feedback-
|
Details | Diff | Splinter Review |
6.37 KB,
patch
|
kk1fff
:
review+
|
Details | Diff | Splinter Review |
While profiling, I added 2 lines to dump the script which was parsed by the JS engine, and dump these to the log, and I also made a script to aggregate them by pid/app-name. I noticed that ErrorPage.js was loaded in all applications (see attachement), but not in the pre-allocated application. Frabice mentionned that this file should only be used for certificate errors. The phone had no wifi setup (at the time of the test), and the date was correctly reset to 09/05/2013 by the flash command.
Comment 1•8 years ago
|
||
Could you attach your scripts to this bug?
Reporter | ||
Comment 2•8 years ago
|
||
Patch used to dump the scripts which are parsed by the JS engine.
Reporter | ||
Comment 3•8 years ago
|
||
Python file which read the dump of adb and present it in a nice way. adb lolcat -s JSFrontend:I > jsfronted.log ./filter-log.py < jsfronted.log
Updated•8 years ago
|
Attachment #800436 -
Attachment mime type: text/x-log → text/plain
Updated•8 years ago
|
Assignee: nobody → ferjmoreno
Comment 4•8 years ago
|
||
With this patch we only inject ErrorPage.js when about:certerror is loaded
Attachment #800811 -
Flags: review?(fabrice)
Updated•8 years ago
|
Attachment #800811 -
Flags: feedback?(pwang)
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 800811 [details] [diff] [review] v1 Review of attachment 800811 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/components/B2GAboutRedirector.js @@ +19,5 @@ > uri: "chrome://browser/content/aboutCertError.xhtml", > privileged: false, > + hide: true, > + preload: function () { > + cpmm.sendAsyncMessage("ErrorPage:Load", {}); The message is async, so it is possible that before the content script has loaded, the error page had already loaded. It could miss the DOMContentLoaded event. I suggest that we also modify the ErrorPage.js to make it check the URI at initialization. ::: b2g/components/ErrorPage.jsm @@ +160,4 @@ > Services.obs.addObserver(this, 'remote-browser-frame-shown', false); > + ppmm.addMessageListener('ErrorPage:Load', (function() { > + try { > + this._mm.loadFrameScript(kErrorPageFrameScript, true); Since the observer is notified each time when a frame is loaded. If we always use the latest messageManager, the script would likely be injected to the newest opened app, instead of the current-shown app. We should load the script to the frame where the message is sent from.
Attachment #800811 -
Flags: feedback?(pwang) → feedback-
Comment 6•8 years ago
|
||
Thanks for your feedback Patrick! (In reply to Patrick Wang [:kk1fff] from comment #5) > Comment on attachment 800811 [details] [diff] [review] > v1 > > Review of attachment 800811 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/components/B2GAboutRedirector.js > @@ +19,5 @@ > > uri: "chrome://browser/content/aboutCertError.xhtml", > > privileged: false, > > + hide: true, > > + preload: function () { > > + cpmm.sendAsyncMessage("ErrorPage:Load", {}); > > The message is async, so it is possible that before the content script has > loaded, the error page had already loaded. It could miss the > DOMContentLoaded event. > I suggest that we also modify the ErrorPage.js to make it check the URI at > initialization. > You are right. I am not sure that I understand what you propose. Can you elaborate more on it, please? What would we do if URI check fails? Couldn't we just wait for a reply from the parent with the confirmation of the frame script injection before creating the new channel? > ::: b2g/components/ErrorPage.jsm > @@ +160,4 @@ > > Services.obs.addObserver(this, 'remote-browser-frame-shown', false); > > + ppmm.addMessageListener('ErrorPage:Load', (function() { > > + try { > > + this._mm.loadFrameScript(kErrorPageFrameScript, true); > > Since the observer is notified each time when a frame is loaded. If we > always use the latest messageManager, the script would likely be injected to > the newest opened app, instead of the current-shown app. We should load the > script to the frame where the message is sent from. You are again right! I guess we'll need to ask the system app to get the currently shown frame.
Comment 7•8 years ago
|
||
Comment on attachment 800811 [details] [diff] [review] v1 I'll update the patch on Sunday.
Attachment #800811 -
Flags: review?(fabrice)
Comment 8•8 years ago
|
||
I've just talked with Fabrice about this and it seems that some docshell hacking might be required in order to fix this issue. I am not familiar with that code, so it might take me a while to figure out how to fix it. Feel free to steal the bug from me.
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo, please) from comment #6) > I am not sure that I understand what you propose. Can you elaborate more on > it, please? What would we do if URI check fails? Couldn't we just wait for a > reply from the parent with the confirmation of the frame script injection > before creating the new channel? Since the initialize process is invoked when the content script is loaded, I think we could do what we do in the handler of DOMContentLoaded in the initialization function. So if the content script was loaded before the error page, we can set the listener of the button's click event when DOMContentLoaded is fired. If the content script is loaded after the error page, we can see that current location become 'about:certerror' and realize we should set the click handler now. Or we could try to prevent newChannel() from returning before we got parent confirm, probably with a nested event loop.
Depends on: 928367
Comment 10•8 years ago
|
||
I open bug 928367 because this file is loaded after the preallocated process has started which in turn slow down app loading by a few ms. I will land my patch in order to save a few ms for now until you found a solution for that.
Updated•8 years ago
|
Assignee | ||
Comment 11•8 years ago
|
||
Another problem is that we don't have information about which frame the error occurs in in AboutRedirector (we only have URI there), so we can't instruct parent to load script into the frame. What I am thinking is to notify all the BrowserElementChilds in the content process when a error occurs, and make all of them load ErrorPage.js. This would make some BrowserElementChilds which in the same process but the error doesn't occur in load ErrorPage.js, but at least it is limited to one process.
Assignee | ||
Comment 12•8 years ago
|
||
Taking advantage of BrowserElement's mozbrowsererror event and loading ErrorPage.js only when the event is fired.
Attachment #820865 -
Flags: review?(fabrice)
Assignee | ||
Updated•8 years ago
|
Assignee: ferjmoreno → pwang
Comment 13•7 years ago
|
||
Comment on attachment 820865 [details] [diff] [review] Patch: Load ErrorPage.js only when error occurs. Review of attachment 820865 [details] [diff] [review]: ----------------------------------------------------------------- sorry for the terrible review delay Patrick. That looks good to me!
Attachment #820865 -
Flags: review?(fabrice) → review+
Comment 14•7 years ago
|
||
I'm flagging this bug for tarako since this will be a small win in terms of preloaded code.
Whiteboard: [c=progress p= s= u=] → [c=progress p= s= u=][tarako]
Assignee | ||
Comment 15•7 years ago
|
||
Thank Fabrice for reviewing this. This is the rebased version.
Attachment #820865 -
Attachment is obsolete: true
Attachment #8356467 -
Flags: review+
Assignee | ||
Comment 16•7 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/8be08de08cc4
Comment 17•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8be08de08cc4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Whiteboard: [c=progress p= s= u=][tarako] → [c=progress p= s=2014.01.17 u=tarako][tarako]
Updated•7 years ago
|
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
Comment 18•7 years ago
|
||
triage: 1.3T+ to have it merged to 1.3T Branch. remove [tarako] from whiteboard
blocking-b2g: --- → 1.3T+
Whiteboard: [c=progress p= s=2014.01.17 u=tarako][tarako] → [c=progress p= s=2014.01.17 u=tarako]
Comment 19•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/8321d68848ff
status-b2g-v1.3T:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•