840823 - Race condition when removing tabs at startup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

When we do an OOM restore startup, we stub tabs in the UI before Gecko actually loads them. If a stub is removed before its tab has been created in Gecko, this causes the stub to be re-added in Java and results in glitchy behavior.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Check whether new tabs were made from stubs to prevent removal race condition

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 713216 [details] [diff] [review]
Check whether new tabs were made from stubs to prevent removal race condition

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>         tabID: this.id,
>         uri: uri,
>         parentId: ("parentId" in aParams) ? aParams.parentId : -1,
>         external: ("external" in aParams) ? aParams.external : false,
>         selected: ("selected" in aParams) ? aParams.selected : true,
>         title: title,
>         delayLoad: aParams.delayLoad || false,
>         desktopMode: this.desktopMode,
>-        isPrivate: isPrivate
>+        isPrivate: isPrivate,
>+        isStub: isStub

At some point we got all Java-ish and started "isXxx". We don't use the pattern for "external" or "selected". Can we change "isStub" and "isPrivate" to "stub" and "private" ? I would accept a second patch for that change.

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 713216 [details] [diff] [review]
> Check whether new tabs were made from stubs to prevent removal race condition
> 
> At some point we got all Java-ish and started "isXxx". We don't use the
> pattern for "external" or "selected". Can we change "isStub" and "isPrivate"
> to "stub" and "private" ? I would accept a second patch for that change.

We may still want to keep "isPrivate" over "private" since "private" is a reserved word in JavaScript: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Reserved_Words

Comment 4

[Brian Nicholson (:bnicholson)]

Pushed with "isStub" renamed to "stub": https://hg.mozilla.org/integration/mozilla-inbound/rev/84f27f437dda

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84f27f437dda

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 713216 [details] [diff] [review]
Check whether new tabs were made from stubs to prevent removal race condition

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 797075
User impact if declined: removing tabs during startup restore will break things
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 7

[Brian Nicholson (:bnicholson)]

Comment on attachment 713216 [details] [diff] [review]
Check whether new tabs were made from stubs to prevent removal race condition

Requesting approval for fx20

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 713216 [details] [diff] [review]
Check whether new tabs were made from stubs to prevent removal race condition

Approving for uplift, low risk and early in beta.

Comment 9

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4bfe9fbede9e