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

VERIFIED FIXED in Firefox 28

Status

P2
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: Dolske, Assigned: kjozwiak)

Tracking

Trunk
Firefox 28
x86
Windows 8.1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [block28] feature=defect c=tbd u=tbd p=1)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Component: General → Browser
OS: Mac OS X → Windows 8 Metro
Whiteboard: [triage]
Version: unspecified → Trunk

Updated

5 years ago
Blocks: 838081

Updated

5 years ago
Summary: Don't use Date.now as an ID → Defect - Don't use Date.now as an ID
Whiteboard: [triage] → feature=defect c=tbd u=tbd p=0

Updated

5 years ago
Whiteboard: feature=defect c=tbd u=tbd p=0 → [release28] feature=defect c=tbd u=tbd p=0
(Assignee)

Comment 1

5 years ago
I will give this one a shot. Assigning it to myself.
Assignee: nobody → kamiljoz
(Assignee)

Comment 2

5 years ago
Created attachment 828436 [details] [diff] [review]
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!
Attachment #828436 - Flags: review?(dolske)
(Reporter)

Comment 3

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

--> mbrubeck since this is Metro code
Attachment #828436 - Flags: review?(dolske) → review?(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();"
Attachment #828436 - Flags: review?(mbrubeck) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 829007 [details] [diff] [review]
Replacing Date.now() with nsIUUIDGenerator

Hi Matt,

Would you mind taking another quick look just to make sure it's correct now? Thanks again!
Attachment #828436 - Attachment is obsolete: true
Attachment #829007 - Flags: review?(mbrubeck)

Updated

5 years ago
Blocks: 931976
No longer blocks: 838081
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] feature=defect c=tbd u=tbd p=0 → [block28] feature=defect c=tbd u=tbd p=1
Attachment #829007 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/2ba06be1165b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Could anyone please give guidelines that can help the QA in verifying this issue? Thanks!
Flags: needinfo?(kamiljoz)
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.
Flags: needinfo?(kamiljoz)
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.
Status: RESOLVED → VERIFIED
Thanks Matt.  I'll remove it from the testing sheet.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.