Last Comment Bug 721577 - Race condition in about:home for tabs from last time
: Race condition in about:home for tabs from last time
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P3 normal (vote)
: Firefox 12
Assigned To: Brian Nicholson (:bnicholson)
:
:
Mentors:
Depends on:
Blocks: 719195
  Show dependency treegraph
 
Reported: 2012-01-26 16:17 PST by Brian Nicholson (:bnicholson)
Modified: 2012-02-06 18:41 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
+


Attachments
patch (2.22 KB, patch)
2012-01-26 16:48 PST, Brian Nicholson (:bnicholson)
mark.finkle: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Brian Nicholson (:bnicholson) 2012-01-26 16:17:43 PST
About:home reads the previous session JSON from sessionstore.js, but sessionstore.js may contain the current session if about:home reads it too late. This isn't much of a problem right now since we always show about:home immediately (meaning sessionstore.js is read before anything is written to it), but this will change when bug 719195 is fixed.
Comment 1 Brian Nicholson (:bnicholson) 2012-01-26 16:48:30 PST
Created attachment 592000 [details] [diff] [review]
patch
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-26 19:28:29 PST
Comment on attachment 592000 [details] [diff] [review]
patch

This looks OK, but I wanted to pitch another thought too: Could we check to see if sessionstore.bak exists and use it. If it doesn't, we could use sessionstore.js ?

The only reason I might like that better is we would be using real file existence as a check, not gecko being ready flag - a flag which does not exactly match up on the session store state.

Thoughts?
Comment 3 Brian Nicholson (:bnicholson) 2012-01-26 22:28:17 PST
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 592000 [details] [diff] [review]
> patch
> 
> This looks OK, but I wanted to pitch another thought too: Could we check to
> see if sessionstore.bak exists and use it. If it doesn't, we could use
> sessionstore.js ?
> 
> The only reason I might like that better is we would be using real file
> existence as a check, not gecko being ready flag - a flag which does not
> exactly match up on the session store state.
> 
> Thoughts?

I originally wanted to do this too, but unfortunately, I don't think that will work. It's possible for sessionstore.bak to have stale data from 2 sessions ago. Consider this sequence:

- have tab google.com open; google.com JSON written to sessionstore.js
- quit
- reopen fennec; google.com JSON moved to sessionstore.bak at session store init
- visit tab cnn.com; cnn.com JSON written to sessionstore.js
- browser crashes
- sessionstore.bak contains google.com JSON rather than cnn.com JSON
Comment 4 Brian Nicholson (:bnicholson) 2012-01-26 22:30:13 PST
Oops - comment collision accidentally killed your flags.
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-26 23:07:46 PST
Comment on attachment 592000 [details] [diff] [review]
patch

OK, lets see where this gets us
Comment 6 Brian Nicholson (:bnicholson) 2012-01-27 17:30:23 PST
Landed on inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/04e40f6991f6
Comment 7 Joe Drew (not getting mail) 2012-01-28 18:59:53 PST
https://hg.mozilla.org/mozilla-central/rev/04e40f6991f6
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-01-29 21:27:17 PST
Brian, please request approval for aurora
Comment 9 Alex Keybl [:akeybl] 2012-02-02 07:04:01 PST
Comment on attachment 592000 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Beta 11.
Comment 10 Brad Lassey [:blassey] (use needinfo?) 2012-02-06 18:41:31 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/cac5504d4637

Note You need to log in before you can comment on or make changes to this bug.