Closed Bug 925771 Opened 6 years ago Closed 6 years ago

[Session Restore] Generated Internal Window IDs Collide Causing Lost Windows

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: smacleod, Assigned: smacleod)

Details

Attachments

(1 file, 1 obsolete file)

SessionStore.jsm generates "unique" identifiers for windows using the following method:

    aWindow.__SSi = "window" + Date.now();

I'm able to cause ID collisions pretty easily by opening windows rapidly at startup using cmd+N. This results in the old window's data being replaced by the new window, and we lose track of the original window. We should switch to generating IDs which are guaranteed unique.
Yay, that's a great find! Can you just use a monotonic counter like we did in bug 586153?
Hardware: x86 → All
This patch switches the ID generation to use an internal counter. This should be sufficient since these IDs are not saved as part of the session, they are just used internally to index data about windows.

Try: https://tbpl.mozilla.org/?tree=Try&rev=1c26d984aeaa
Attachment #817568 - Flags: review?(ttaubert)
Comment on attachment 817568 [details] [diff] [review]
Patch - Generate Consecutive Window IDs Using a Counter

Review of attachment 817568 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +690,5 @@
> +   * @return string
> +   *         A unique string to identify a window
> +   */
> +  _generateWindowID: function ssi_generateWindowID() {
> +    return "window" + (this._nextWindowID++);

Super nit: can we make this 'window-123'?

@@ +714,5 @@
>      if (aWindow.document.documentElement.getAttribute("windowtype") != "navigator:browser" ||
>          this._loadState == STATE_QUITTING)
>        return;
>  
> +    // assign it a unique identifier

Nit: can you please improve the comment a little bit?

@@ +892,5 @@
>      // this window was about to be restored - conserve its original data, if any
>      let isFullyLoaded = this._isWindowLoaded(aWindow);
>      if (!isFullyLoaded) {
>        if (!aWindow.__SSi)
> +        aWindow.__SSi = this._generateWindowID();

Nit: while you're here, add brackets :)
Attachment #817568 - Flags: review?(ttaubert) → review+
(In reply to Tim Taubert [:ttaubert] (limited availability, work week) from comment #3)
> > +    return "window" + (this._nextWindowID++);
> 
> Super nit: can we make this 'window-123'?

I was keeping the old format on the crazy chance that someone is using these IDs, and relying on their format. While it's probably very unlikely someone is relying on this, should we change it if we don't have to? What is the reason you'd like the dash added?
Unique IDs in other places tend to have dashes between words and numbers in other places so I thought it might be a good idea to stick with that. But if compatibility is a concern then please keep it, I really don't feel strongly.
Updated patch to fix the issues Tim brought up.
Attachment #817568 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 817891 [details] [diff] [review]
Patch - Generate Consecutive Window IDs Using a Counter v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Session Store generates internal window identifiers using Date.now(), which if two windows are generated close in time can cause collisions.

User impact if declined: If two windows are opened in rapid succession they may have the same ID, causing session data loss.

Testing completed (on m-c, etc.): No breakages on Try

Risk to taking this patch (and alternatives if risky): very low risk, simple patch just switched Date.now() calls with a counter that is incremented.

String or IDL/UUID changes made by this patch: None
Attachment #817891 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1b53a6a4d2de
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Reopening since I'd forgotten [leave open], and would like this uplifted to aurora
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
(In reply to Steven MacLeod [:smacleod] from comment #10)
> Reopening since I'd forgotten [leave open], and would like this uplifted to
> aurora
Was operating under the incorrect assumption this was the proper way to handle things. oops.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Attachment #817891 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
I get the same results when trying to reproduce this on Firefox 25.0 (release), Firefox 26b1 and Firefox 27a2 (10/31):

Steps:
I open several windows fast with Ctrl+N.
Sometimes I loaded different pages in the windows.
I closed Firefox, then restarted it.

Results:
Only 3 windows are restored: the main one (window opened when I first started Firefox), the first window opened with Ctrl+N and one more window.


I tested this with different new and old profiles and I get the exact same results each time. The whole testing was done on Ubuntu 13.04 32bit.
Could what I described in comment 13 be a different bug? or is this bug not completely fixed?
Flags: needinfo?(smacleod)
(In reply to Ioana Budnar, QA [:ioana] from comment #14)
> Could what I described in comment 13 be a different bug? or is this bug not
> completely fixed?
I'm fairly certain this bug is fixed, there shouldn't be any more collisions of the ids with how we generate them now. If windows are still being lost after both this, and Bug 900910 were fixed, there might be some other new problem causing the windows to be lost.
Flags: needinfo?(smacleod)
(In reply to Steven MacLeod [:smacleod] from comment #15)
> (In reply to Ioana Budnar, QA [:ioana] from comment #14)
> > Could what I described in comment 13 be a different bug? or is this bug not
> > completely fixed?
> I'm fairly certain this bug is fixed, there shouldn't be any more collisions
> of the ids with how we generate them now. If windows are still being lost
> after both this, and Bug 900910 were fixed, there might be some other new
> problem causing the windows to be lost.

Filed bug 947159 for this issue. I tried to reproduce/verify the bug in this report by closing the browser with File>Quit, but I couldn't reproduce it.

If anyone here could reproduce this specific bug, please try to verify it on the branches with the fix.
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.