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

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Steven MacLeod [:smacleod]]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Yay, that's a great find! Can you just use a monotonic counter like we did in bug 586153?

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: Patch - Generate Consecutive Window IDs Using a Counter

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

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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 :)

Comment 4

[Steven MacLeod [:smacleod]]

(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?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 6

[Steven MacLeod [:smacleod]]

Attachment: Patch - Generate Consecutive Window IDs Using a Counter v2

Updated patch to fix the issues Tim brought up.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/1b53a6a4d2de

Comment 8

[Steven MacLeod [:smacleod]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b53a6a4d2de

Comment 10

[Steven MacLeod [:smacleod]]

Reopening since I'd forgotten [leave open], and would like this uplifted to aurora

Comment 11

[Steven MacLeod [:smacleod]]

(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.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/838fe637ae0c

Comment 13

[Ioana (away)]

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.

Comment 14

[Ioana (away)]

Could what I described in comment 13 be a different bug? or is this bug not completely fixed?

Comment 15

[Steven MacLeod [: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.

Comment 16

[Ioana (away)]

(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.