Closed Bug 793082 Opened 7 years ago Closed 7 years ago

Allow sendChromeEvent to queue the events until window.onload fires

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(blocking-basecamp:+)

RESOLVED FIXED
blocking-basecamp +

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
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?
Also, the nothing was not broken on the otoro or on desktop today for me. Which gaia changeset are you using?
(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).
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).
(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.
... 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.
Philikon, do you think this can replace the event in bug 787172 too?
(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.
Tim, since it seems you're investigating, can you own this?
Assignee: nobody → timdream+bugs
blocking-basecamp: ? → +
Summary: The webapps-registry-ready mozChromeEvent can fire before System app is loaded → Allow sendChromeEvent to queue the events until window.onload fires
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #663463 - Flags: review?
Attachment #663462 - Attachment is obsolete: true
Attachment #663284 - Attachment is obsolete: true
After this patch is landed, there will be follow up bugs that proceed to remove approaches did in bug 787172 and bug 783149.
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.
Attachment #663463 - Flags: review? → review+
https://hg.mozilla.org/mozilla-central/rev/f34f3d0bd13d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I had to backout because this was breaking the homescreen loading on desktop builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7843d95718a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Patch v2Splinter Review
There is already a |let content| on line 291 of the same function :-/
Attachment #663725 - Flags: review?(fabrice)
line 269 I meant ...
Attachment #663463 - Attachment is obsolete: true
Attachment #663646 - Attachment is obsolete: true
Attachment #663725 - Flags: review?(fabrice) → review+
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()
re-pushed since this was not the cause of test failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d34057e17d8
https://hg.mozilla.org/mozilla-central/rev/3d34057e17d8
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.