909124 - [b2g desktop] shell.js did not get correct mozbrowserloadstart event to send Gaia the pending chromeEvents

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

STR
====

After 3rd-party IME framework has landed to Gaia master, bc8c1c962addd419b48ba12c6d8ddabdf9805391, we found that it would fail to run Gaia with b2g desktop.

It would be stuck at the init logo.

Comment 1

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

When this issue can be reproduced, I noticed that Gaia system app would not received "webapps-registry-ready" chromeEvent from Gecko to start loading the FTU app.

We do mozApps.mgmt.getAll() in shared/js/keyboard_helper.js, which is invoked from keyboard_manager.js, I suspect this might be the root cause.

I am trying to open debug log of Gecko's mozApps API to see what happened.

Comment 2

[Kevin Grandon :kgrandon]

My comment in the email: I would suggest modifying and building b2g desktop in a way where it includes our desktop shims.

Telephony shim: https://github.com/mozilla-b2g/gaia/blob/master/tools/extensions/desktop-helper/content/data/lib/telephony.js

Comment 3

[James Lal [:lightsofapollo]]

:rudyl++ I can confirm this is not the lockscreen error mentioned in the mailing list... That has been present for the last month without any obvious breakage.

Comment 4

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I am tracing to b2g/chrome/content/shell.js, when this issue occurs, `shell.isHomeLoaded` won't be set as true, so Gaia would not receive the chromeEvents for "webapps-registry-ready".

Still trying to figure out why `shell.isHomeLoaded` won't be set as true.
(Due to the 'load' event was not emitted from content?)

Comment 5

[Yuan Xulei [:yxl] (Leave from May 18)]

Hi James,

We need logs to identify the problem.

Could you provide the js log when error occurs by running b2g-desktop with `-jsconsole` parameter. For example on ubuntu 12.x, I run b2g-destop with the following command:

 b2g -profile $GAIA_PROFILE -jsconsole

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

I'm building custom desktop b2g to diagnose this.
BTW,
1. Don't go through mgmt.getAll before applicationready.
2. I see load event is correctly fired.
3. It seems sometimes cannot be reproduced...timing issue? Race condition?

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

[Investigation Result]
I found that the root cause might be in mozbrowser API event.

In shell.js
http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#465

We will wait for mozbrowserloadstart event and in that event handler, trying to listen for content's load event.
However, when this issue occurs, somehow it would receive 2 times of mozbrowserloadstart event with document.location == "about:blank".
So it never add event listener for content load event.


[Potential workaround]
We might be able to listen to mozbrowserlocationchange and then add event listener to content load.

Still need more time to test if this workaround can work on device and ff nightly.

Comment 8

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

[Root Cause]
When running Gaia with b2g desktop, most of the time, shell.js would get mozbrowserloadstart event with document.location == "about:blank", so it did not add event listener for Gaia's onload, so Gais would be stuck at waiting for Gecko's chromeEvent.

Before 3rd-party IME has landed, bc8c1c962addd419b48ba12c6d8ddabdf9805391, this would still work because we would load keyboard iframe immediately after loading Gaia system app, and this will generate additional mozbrowserloadstart for shell.js to do the follow-up init process.

After 3rd-party IME landed, we would delay keyboard iframe loading until we got the chromeEvent of 'webapps-registry-ready' from Gecko, so the above "magic" from loading keyboard iframe won't happen again.

I have a patch to workaround this.

Comment 9

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Bug909124.patch

Hi Fabrice, Tim,

This patch is to workaround the srange event sequences of mozbrowserloadstart by listening to mozbrowserlocationchange.

If we get 'mozbrowserloadstart' and found its document.location is still "about:blank", then we could expect there will be a mozbrowserlocationchange event to come.

Please help review this.
Thank you.

Comment 10

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Bug909124.patch - V1.1

Slightly modify the patch.
Will remove the debug logs after review.

Thanks.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 795444 [details] [diff] [review]
Bug909124.patch - V1.1

I am not qualified of reviewing B2G runtime.

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 795444 [details] [diff] [review]
Bug909124.patch - V1.1

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

r=me but I'd like to see the final version with all the debug logs removed.

::: b2g/chrome/content/shell.js
@@ +327,5 @@
>      window.removeEventListener('MozApplicationManifest', this);
>      window.removeEventListener('mozfullscreenchange', this);
>      window.removeEventListener('sizemodechange', this);
>      this.contentBrowser.removeEventListener('mozbrowserloadstart', this, true);
> +    this.contentBrowser.removeEventListener('mozbrowserlocationchange', this, true);

You already remove these event listeners in onContentStart.

@@ +584,5 @@
>          let sender = message.target.QueryInterface(Ci.nsIMessageSender);
>          sender.sendAsyncMessage(activity.response, { success: false });
>        }
>      }
> +  },

nit: add blank line.

Comment 13

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Bug909124.patch - V1.2

Carry forward the r+.

Fabrice, Tim,

Thanks for the review.

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Bug909124.patch - V1.2

Add r=fabrice to the commit message.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/294fde226b78

Comment 16

[Ed Morley [:emorley]]

Backed out for causing:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27117512&tree=B2g-Inbound

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/618b30fe6337

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Re-landed. The bustage wasn't from this patch.
https://hg.mozilla.org/integration/b2g-inbound/rev/82d22aca8337

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82d22aca8337