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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: 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.

Comment 1

[[:fabrice] Fabrice Desré]

Could you attach your scripts to this bug?

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: parsed-scripts.patch

Patch used to dump the scripts which are parsed by the JS engine.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: 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

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

With this patch we only inject ErrorPage.js when about:certerror is loaded

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

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

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 800811 [details] [diff] [review]
v1

I'll update the patch on Sunday.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 9

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(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.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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.

Comment 11

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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.

Comment 12

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Load ErrorPage.js only when error occurs.

Taking advantage of BrowserElement's mozbrowsererror event and loading ErrorPage.js only when the event is fired.

Comment 13

[[:fabrice] Fabrice Desré]

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!

Comment 14

[[:fabrice] Fabrice Desré]

I'm flagging this bug for tarako since this will be a small win in terms of preloaded code.

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: Load ErrorPage.js only when error occurs.

Thank Fabrice for reviewing this. This is the rebased version.

Comment 16

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/b2g-inbound/rev/8be08de08cc4

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8be08de08cc4

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ to have it merged to 1.3T Branch. remove [tarako] from whiteboard

Comment 19

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/8321d68848ff