Closed Bug 1003962 Opened 11 years ago Closed 11 years ago

Line Runner hangs on launch with white screen

Categories

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Product:
Firefox for Android Graveyard
Component:
Web Apps (PWAs)
Version:
29 Branch
Platform:
ARM
Android
Type:
defect

Tracking

(firefox32 verified, firefox33 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 33
Tracking Flags:
Tracking Status
firefox32 --- verified
firefox33 --- verified

People

(Reporter: myk, Assigned: jhugman)

References

Details

(Whiteboard: [WebRuntime])

Attachments

(3 files, 3 obsolete files)

bug-1003962.apk
49.14 KB, application/zip
Details
bug-1003962-5.patch
1.76 KB, patch
myk
: review+
kats
: feedback+
Details | Diff | Splinter Review
patch that landed on Central
2.25 KB, patch
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
The Line Runner app <https://marketplace.firefox.com/app/line-runner> hangs on launch with a white screen. The problem occurs on all versions I've tested (today's Nightly, Beta, and Release). Logs are uninformative.
Assignee: nobody → jhugman
Attached file bug-1003962.apk —
jhugman and I investigated this today and figured out that there's a race on startup between loading of the app's launch page and Tab.setBrowserSize. Line Runner's launch page sets Width and Height variables to window.innerWidth/innerHeight as soon as possible while loading, via a <script> tag in its <head> section. And Tab.setBrowserSize gets called four times while loading the launch page tab, the first of which happens before the launch page has started loading. But Tab.setBrowserSize only calls nsIDOMWindowUtils.setCSSViewport if this.browser.contentWindow, so it doesn't call that function the first time. And the second time is already too late: by then, Line Runner's launch page has already loaded enough to call that script, which sets its Width and Height variables to zero. Here's a minimal test case that demonstrates the problem. It's a packaged app you can sideload with adb. Its code lives at <https://github.com/mykmelez/bug-1003962>. And it does only one thing: log innerWidth/innerHeight as soon as possible: > <!DOCTYPE html> > > <html> > <head> > <script> > console.log("window.innerWidth/innerHeight: " + window.innerWidth + "/" + window.innerHeight); > </script> > <title>Bug 1003962</title> > <meta charset="utf-8"> > </head> > > <body> > </body> > </html> The first time you start the app after sideloading it, the values are correct! Presumably this is because firstrun of a sideloaded app has to register the app with DOMApplicationRegistry, which delays launch long enough to avoid the race. But the second and subsequent times, the app will log: window.innerWidth/innerHeight: 0/0 mfinkle: setBrowserSize gets called by Tab.create the first time, but it looks like subsequent calls come from Tab.prototype.observe, which observes before-first-paint from Gecko, or ViewportHandler.observe, which observes Window:Resize from GeckoLayerClient.java. So I suspect a platform race (or perhaps the JavaScript code needs to observe some earlier event as well). In any case, do you have a suggestion for someone who might be able to help figure out if there's a platform race here?
Flags: needinfo?(mark.finkle)
Kats might be able to shed some light on things. Wes has played around in some of these areas too, but I think Kats is our first choice.
Flags: needinfo?(wjohnston)
Flags: needinfo?(mark.finkle)
Flags: needinfo?(bugmail.mozilla)
This code is has been a source of problems like this for a while. In this case I *think* the right solution might be to call this.setBrowserSize(kDefaultCSSViewportWidth, kDefaultCSSViewportHeight); inside this block: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=5be519918f65#4155 I suspect that will fix this problem but I have no idea if it will break other things.
Flags: needinfo?(bugmail.mozilla)
Attachment #8450148 - Flags: review?(myk)
Attachment #8450148 - Flags: review?(mark.finkle)
Fixes both LineRunner and Myk's test apk. > window.innerWidth/innerHeight: 980/480
Comment on attachment 8450148 [details] [diff] [review] Calls setBrowserSize just before content js is run. >+ // XXX Bug 1003962. Under some circumstances, the browser size isn't set before >+ // content javascript begins to execute. Setting this here ensures it is set. >+ // If the internal state of this object thinks it's already been set, then the >+ // method will exit early, before actually doing anything. >+ this.setBrowserSize(0, 0); >+ this.setBrowserSize(kDefaultCSSViewportWidth, kDefaultCSSViewportHeight); > this.contentDocumentIsDisplayed = false; 1. Do you need to call setBrowserSize twice (once with 0, 0) or is the default width and height enough? 2. I'd add a blank line after the setBrowserSize block to set it apart from the normal flow. f+ from me, but check #1 to see if we can minimize the calls a bit and I want Kats to take a look too.
Attachment #8450148 - Flags: review?(mark.finkle)
Attachment #8450148 - Flags: review?(bugmail.mozilla)
Attachment #8450148 - Flags: feedback+
Calling setBrowserSize once with the default sizes doesn't have the desired effect. Alternatives to this would be to set this.browserHeight or this.browserWidth directly.
Comment on attachment 8450148 [details] [diff] [review] Calls setBrowserSize just before content js is run. Review of attachment 8450148 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +4159,5 @@ > + // content javascript begins to execute. Setting this here ensures it is set. > + // If the internal state of this object thinks it's already been set, then the > + // method will exit early, before actually doing anything. > + this.setBrowserSize(0, 0); > + this.setBrowserSize(kDefaultCSSViewportWidth, kDefaultCSSViewportHeight); Replace the (0, 0) call with a new aForce boolean parameter to setBrowserSize. If aForce is true then bypass the width/height check. Update this comment to indicate that we need to force-set the new CSS viewport because we have a new browser and new window, so the old browserWidth and browserHeight are no longer valid. Also, remove all the trailing whitespace here.
Attachment #8450148 - Flags: review?(bugmail.mozilla) → review-
Comment on attachment 8450148 [details] [diff] [review] Calls setBrowserSize just before content js is run. Review of attachment 8450148 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/browser.js @@ +4154,5 @@ > if (!sameDocument) { > // XXX This code assumes that this is the earliest hook we have at which > // browser.contentDocument is changed to the new document we're loading > + > + // XXX Bug 1003962. Under some circumstances, the browser size isn't set before Don't prefix this comment with "XXX", as that implies that there's more to do here, whereas your fix is actually complete and (presumably) permanent. Nit: also, I wouldn't reference the bug number in this case, since the fix will resolve the bug (so there isn't an unresolved issue to reference by pointing at the bug), and its comment provides enough context (so there isn't additional information that future hackers will need to learn by looking at the bug).
Attachment #8450148 - Flags: review?(myk) → review-
Attached patch bug-1003962-2.patch (obsolete) — — Splinter Review
Suggested comments acted upon.
Attachment #8450148 - Attachment is obsolete: true
Attachment #8451859 - Flags: review?(myk)
Attachment #8451859 - Flags: feedback?(bugmail.mozilla)
Flags: needinfo?(wjohnston)
Comment on attachment 8451859 [details] [diff] [review] bug-1003962-2.patch Review of attachment 8451859 [details] [diff] [review]: ----------------------------------------------------------------- Patch file is busted. It has two patches that need to be squashed together.
Attachment #8451859 - Flags: feedback?(bugmail.mozilla)
Attached patch Single patch, multiple commits. (obsolete) — — Splinter Review
Attachment #8451859 - Attachment is obsolete: true
Attachment #8451859 - Flags: review?(myk)
Attachment #8452230 - Flags: review?(myk)
Attachment #8452230 - Flags: feedback?(lists.bugzilla)
Comment on attachment 8452230 [details] [diff] [review] Single patch, multiple commits. Review of attachment 8452230 [details] [diff] [review]: ----------------------------------------------------------------- Wrong email address
Attachment #8452230 - Flags: feedback?(lists.bugzilla) → feedback?(bugmail.mozilla)
Comment on attachment 8452230 [details] [diff] [review] Single patch, multiple commits. Review of attachment 8452230 [details] [diff] [review]: ----------------------------------------------------------------- This patch still seems "wrong" in that it won't apply cleanly to master. But anyway. ::: mobile/android/chrome/content/browser.js @@ +4160,5 @@ > > + // Under some circumstances, the browser size isn't set before > + // content javascript begins to execute. Setting this here ensures it is set. > + this.setBrowserSize(kDefaultCSSViewportWidth, kDefaultCSSViewportHeight, true); > + Remove trailing whitespace bits here. Also update the comment so that it explicitly says we have a new browser window object which doesn't have a CSS viewport set, which is why we need to set it.
Attachment #8452230 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 8452230 [details] [diff] [review] Single patch, multiple commits. Review of attachment 8452230 [details] [diff] [review]: ----------------------------------------------------------------- It looks like this is a patch on top of your earlier patch, whereas it needs to be a patch against master, so reviewers and landers can apply it cleanly without having to dig up and apply the other patch first. Otherwise, I don't have any additional comments besides what kats said.
Attachment #8452230 - Flags: review?(myk) → review-
Patch addresses feedback.
Attachment #8452230 - Attachment is obsolete: true
Attachment #8453078 - Flags: review?(myk)
Attachment #8453078 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8453078 [details] [diff] [review] bug-1003962-5.patch Review of attachment 8453078 [details] [diff] [review]: ----------------------------------------------------------------- Contents of the patch look fine, except for the trailing whitespace. I'd also recommend adding this (or equivalent) to your .gitconfig: [alias] hgp = "show -M -C --binary --format=\"# HG changeset patch%n# User %an <%ae>%n%B\" -U8" and then exporting the patch using "git hgp <cset>" rather than generating mail-formatted patches which are harder to read and contain unnecessary header junk. ::: mobile/android/chrome/content/browser.js @@ +4157,4 @@ > if (!sameDocument) { > // XXX This code assumes that this is the earliest hook we have at which > // browser.contentDocument is changed to the new document we're loading > + nit: trailing ws
Attachment #8453078 - Flags: feedback?(bugmail.mozilla) → feedback+
Comment on attachment 8453078 [details] [diff] [review] bug-1003962-5.patch Review of attachment 8453078 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I second kats's nit about the whitespace and add one of my own about the comment! ::: mobile/android/chrome/content/browser.js @@ +4158,5 @@ > // XXX This code assumes that this is the earliest hook we have at which > // browser.contentDocument is changed to the new document we're loading > + > + // We have a new browser and a new window, so the old browserWidth and > + // browserHeight are no longer valid. We need to force-set the new CSS viewport. Nit: It'd be helpful to future readers of this code for this comment to explain *why" we need to force-set the new CSS viewport.
Attachment #8453078 - Flags: review?(myk) → review+
James is out today, so I've pushed this to fx-team with the nits addressed in the interest of landing it sooner than later. https://hg.mozilla.org/integration/fx-team/rev/f3f1735c4c09
https://hg.mozilla.org/mozilla-central/rev/f3f1735c4c09
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
I've seen this with a couple of apps, including Penguin Pop. I was trying to understand how the process state impacted this, and concluded that it didn't -- so it's nice to see an alternate explanation.
This is the patch I landed on Central. Requesting approval to uplift to Aurora. Approval Request Comment [Feature/regressing bug #]: No known bug number. [User impact if declined]: Users who install Line Runner and certain other apps won't be able to use them, because they'll stall on launch at a white screen of sorrow. [Describe test coverage new/current, TBPL]: This landed five days ago on Central, and it's been baking since then without any apparent regressions. [Risks and why]: This seems low-risk, as it is well-contained and merely calls some code more often than it would otherwise be called. That code does, however, get called when tabs load in Fennec in addition to within a webapp process, so it's possible for this patch to affect tab load. That should show up in automated tests, however, which hasn't happened. [String/UUID change made/needed]: None.
Attachment #8457023 - Flags: approval-mozilla-aurora?
Attachment #8457023 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on LG Nexus 4(Android 4.4.4) on Firefox 32 RC and on latest Aurora
Status: RESOLVED → VERIFIED
status-firefox32: fixed → verified
status-firefox33: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: