927038 - Defect - Don't use Date.now as an ID

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P2)

Description

[Justin Dolske [:Dolske]]

We've fixed a number of bugs (bug 925771, bug 586153, bug 578589) that were a result of using Date.now() as an identifier. The problem is that Date.now() can return the same value if called again in a sufficiently short period of time. [In the past Windows was the worst, as the result only changed ~60Hz].

I see a use of this in Metro:

browser/metro/base/content/bindings/browser.js

239   _deserializeHistoryEntry: function ...
...
258     if (aEntry.ID) {
259       // get a new unique ID for this frame (since the one from the last
260       // start might already be in use)
261       let id = aIdMap[aEntry.ID] || 0;
262       if (!id) {
263         for (id = Date.now(); id in aIdMap.used; id++);
264         aIdMap[aEntry.ID] = id;
265         aIdMap.used[id] = true;
266       }
267       shEntry.ID = id;
268     }

This seems especially sketchy since the loop in incrementing the timestamp, making a collision more likely in a future call.

It would be safer to use a simple static counter, or even nsIUUIDGenerator.

Comment 1

[Kamil Jozwiak [:kjozwiak]]

I will give this one a shot. Assigning it to myself.

Comment 2

[Kamil Jozwiak [:kjozwiak]]

Attachment: Replacing Date.now() with nsIUUIDGenerator

Replaced Date.now() with nsIUUIDGenerator as suggested in comment #0.

Justin, would you mind taking a look and seeing if its done correctly? Thanks!

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 828436 [details] [diff] [review]
Replacing Date.now() with nsIUUIDGenerator

--> mbrubeck since this is Metro code

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 828436 [details] [diff] [review]
Replacing Date.now() with nsIUUIDGenerator

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

Thanks!  One minor change suggested below to avoid a possible performance trap:

::: browser/metro/base/content/bindings/browser.js
@@ +259,5 @@
>        // get a new unique ID for this frame (since the one from the last
>        // start might already be in use)
>        let id = aIdMap[aEntry.ID] || 0;
>        if (!id) {
> +        id = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator).generateUUID();

To avoid calling getService repeatedly, please add a global getter near the top of this file, like this:

XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
  "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator");

and then in this line you can write "id = gUUIDGenerator.generateUUID();"

Comment 5

[Kamil Jozwiak [:kjozwiak]]

Attachment: Replacing Date.now() with nsIUUIDGenerator

Hi Matt,

Would you mind taking another quick look just to make sure it's correct now? Thanks again!

Comment 6

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2ba06be1165b

Comment 7

[Brian R. Bondy [:bbondy]]

Passed try here by the way:
https://tbpl.mozilla.org/?tree=Try&rev=9f2725af60ba

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ba06be1165b

Comment 9

[Manuela Muntean [Away]]

Could anyone please give guidelines that can help the QA in verifying this issue? Thanks!

Comment 10

[Brian R. Bondy [:bbondy]]

I think just go through the bugs listed in Comment 0 and verify that similar problems do not exist.  You can also just check for regressions around history.

Comment 11

[Matt Brubeck (:mbrubeck)]

I don't think there was any user-visible bug here, since the old code *was* careful not to reuse IDs.  This is just a code cleanup issue.

Comment 12

[Marco Mucci [:MarcoM]]

Thanks Matt.  I'll remove it from the testing sheet.