Closed Bug 913243 Opened 7 years ago Closed 6 years ago

All applications are loading chrome://browser/content/ErrorPage.js

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

RESOLVED FIXED
1.3 C2/1.4 S2(17jan)
blocking-b2g 1.3T+
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)

Attached file frontend.3.nice.log
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.
Could you attach your scripts to this bug?
Patch used to dump the scripts which are parsed by the JS engine.
Attached file filter-log.py
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
Attachment #800436 - Attachment mime type: text/x-log → text/plain
Assignee: nobody → ferjmoreno
Attached patch v1Splinter Review
With this patch we only inject ErrorPage.js when about:certerror is loaded
Attachment #800811 - Flags: review?(fabrice)
Attachment #800811 - Flags: feedback?(pwang)
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-
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 on attachment 800811 [details] [diff] [review]
v1

I'll update the patch on Sunday.
Attachment #800811 - Flags: review?(fabrice)
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.
(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.
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.
Status: NEW → ASSIGNED
Keywords: perf
Whiteboard: [c=progress p= s= u=]
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.
Taking advantage of BrowserElement's mozbrowsererror event and loading ErrorPage.js only when the event is fired.
Attachment #820865 - Flags: review?(fabrice)
Assignee: ferjmoreno → pwang
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+
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]
Thank Fabrice for reviewing this. This is the rebased version.
Attachment #820865 - Attachment is obsolete: true
Attachment #8356467 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8be08de08cc4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [c=progress p= s= u=][tarako] → [c=progress p= s=2014.01.17 u=tarako][tarako]
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
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]
You need to log in before you can comment on or make changes to this bug.