793082 - Allow sendChromeEvent to queue the events until window.onload fires

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

Today's build is broken on Otoro due to the fact launchHomescreen() is never triggered and home screen is never loaded. Upon investigation I found the what breaks B2G Desktop now happens on Otoro too.

Remove the event listener and replaces it with a 5 sec timer can workaround this temporary, but we need a real fix in Gecko.
http://pastebin.mozilla.org/1833801

Depend on our schedule, we got some options here:

1. Make sure webapps-registry-ready mozChromeEvent fire after window.onload of the System app. (can be easily done in shell.js
2. Get rid of the event. I am not familiar why we need this actually; Can I say, naïvely, that since both mozApps.mgmt.getAll() and |new MozActivity()| are async requests, we can just put the onsuccess/onerror callbacks on hold and only start process it when registry is ready right? I have no idea how much effort does it takes though.

Comment 1

[[:fabrice] Fabrice Desré]

For 1., I'm really puzzled that webapps-registry-ready fires too early. We load Webapps.jsm only after 'contentstart', and webapps-registry-ready after that. Why is the system not loaded at this point?

Comment 2

[[:fabrice] Fabrice Desré]

Also, the nothing was not broken on the otoro or on desktop today for me. Which gaia changeset are you using?

Comment 3

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

(In reply to Fabrice Desré [:fabrice] from comment #1)
> For 1., I'm really puzzled that webapps-registry-ready fires too early. We
> load Webapps.jsm only after 'contentstart', and webapps-registry-ready after
> that. Why is the system not loaded at this point?

ContentStart event happens at |mozbrowserloadstart|, which is way before window.onload and in the middle of script loading. We can change the occasion of that event instead of my patch (follows).

Comment 4

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

Attachment: Patch to save all ChromeEvents in a queue

In this patch, right after |mozbrowserloadstart|, we attach an load event listener to the system app. All |sendChromeEvent| calls before system app loads will be put into a |pendingChromeEvents| queue.

This is the generalized implementation of solution (1).

Comment 5

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #3)
> ContentStart event happens at |mozbrowserloadstart|, which is way before
> window.onload and in the middle of script loading. We can change the
> occasion of that event instead of my patch (follows).

With a second look I found the we cannot change the time ContentStart fires. The |CustomEventManager.init| need that event to know when to attach event listener of |mozContentEvent|, which must be attached before |window.onload|.

Anyhow, my point is to make |window.onload| of System app the exact moment, where

- event listeners of shell.js and System app must be attached _before_ that
- event triggering of shell.js and System app must be attached _after_ that

This should fix most of our racing nightmare between two.

Comment 6

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

... and we can get rid of that the sad 10 sec timeout introduced in bug 783149 too since we know when window.onload took place how.

Comment 7

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

Philikon, do you think this can replace the event in bug 787172 too?

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #7)
> Philikon, do you think this can replace the event in bug 787172 too?

Probably, yeah.

Comment 9

[Andrew Overholt [:overholt]]

Tim, since it seems you're investigating, can you own this?

Comment 10

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

Attachment: Bug 793082 - Allow sendChromeEvent to queue the events until window.onload fires

Comment 11

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

Attachment: patch v1

Comment 12

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

After this patch is landed, there will be follow up bugs that proceed to remove approaches did in bug 787172 and bug 783149.

Comment 13

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

https://github.com/mozilla-b2g/gaia/pull/4721 would need some re-touch too.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 663463 [details] [diff] [review]
patch v1

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

r=me with nits addressed.

::: b2g/chrome/content/shell.js
@@ +302,5 @@
>          DOMApplicationRegistry.allAppsLaunchable = true;
>  
>          this.sendEvent(window, 'ContentStart');
> +
> +        var content = this.contentBrowser.contentWindow;

s/var/let

@@ +308,5 @@
> +          content.removeEventListener('load', shell_homeLoaded);
> +          shell.isHomeLoaded = true;
> +
> +          if ('pendingChromeEvents' in shell)
> +            shell.pendingChromeEvents.forEach((shell.sendChromeEvent).bind(shell));

use { ... } even for a single-line if.

@@ +363,5 @@
> +        this.pendingChromeEvents = [];
> +      }
> +
> +      this.pendingChromeEvents.push(details);
> +

Nit: remove this blank line.

Comment 15

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

Attachment: Patch to commit

Comment 16

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f34f3d0bd13d

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f34f3d0bd13d

Comment 18

[[:fabrice] Fabrice Desré]

I had to backout because this was breaking the homescreen loading on desktop builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7843d95718a

Comment 19

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

Attachment: Patch v2

There is already a |let content| on line 291 of the same function :-/

Comment 20

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

line 269 I meant ...

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/b7843d95718a

Comment 22

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddee22deae16

Comment 23

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/25041824911b - one of the things in that push was causing

9351 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_cross_origin.xul | Test timed out.
9354 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_getNotInstalled.xul | Test timed out.
9358 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_app.xul | Test timed out.
9364 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul | Test timed out.
9365 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test timeouts, giving up.
9366 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | Skipping 404 remaining tests.
9367 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | chrome://mochitests/content/chrome/dom/tests/mochitest/webapps/test_install_errors.xul finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()

Comment 24

[[:fabrice] Fabrice Desré]

re-pushed since this was not the cause of test failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d34057e17d8

Comment 25

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/3d34057e17d8