Race condition when removing tabs at startup

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

unspecified
Firefox 21
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19 affected, firefox20 fixed, firefox21 fixed)

Details

Attachments

(1 attachment)

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 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.
Attachment #713216 - Flags: review?(mark.finkle) → review+
Assignee

Comment 3

6 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/84f27f437dda
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Assignee

Comment 6

6 years ago
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
Attachment #713216 - Flags: approval-mozilla-aurora?
Assignee

Comment 7

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

Requesting approval for fx20
Attachment #713216 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
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.
Attachment #713216 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.