Closed Bug 586153 Opened 9 years ago Closed 6 years ago

unique panel ID shouldn't use Date.now()

Categories

(Firefox :: General, defect)

1.5.0.x Branch
x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 27
Tracking Status
firefox25 --- wontfix
firefox26 --- verified
firefox27 --- verified

People

(Reporter: Dolske, Assigned: ttaubert)

References

Details

(Keywords: verifyme)

Attachments

(1 file)

No description provided.
FUUUUUU pressed enter and bugzilla submitted. H8.

Anyway, this code is in tabbrowser.xml's addTab():

1158             var uniqueId = "panel" + Date.now() + position;

Seems like this could be prone to fail if tabs are added quickly, and run into Windows' granular timer? Seems like a monotonic counter would be safer.
Dates are the wrong source of unique IDs on all platforms for a number of reasons - a monotonic counter is the way to go.
Justin, does the unique ID have to be unique across all windows or just one window? (I'm guessing the latter.)
They likely only need to be unique in a given window (to avoid multiple elements in the document having the same ID), but given that they've also always been globally unique (or mostly so, anyways :/), we probably don't want to change that - it's possible (and perhaps even likely) that people are depending on that.
Yeah, seems safest to keep them globally unique. You could probably just append a tab counter to the window ID to do so?
(In reply to comment #5)
> Yeah, seems safest to keep them globally unique. You could probably just append
> a tab counter to the window ID to do so?

We have window IDs? Where do I find them?
nsIDOMWindowUtils.outerWindowID is what I'm thinking of.
Assignee: nobody → ttaubert
Blocks: 903834
Status: NEW → ASSIGNED
OS: Windows 7 → Windows XP
I investigated bug 903834 and pushed a cset to try [1] that includes some debug output that checks if there's a unique ID collision. Turns out there sometimes is and it makes the whole m-bc run fail:

https://tbpl.mozilla.org/php/getParsedLog.php?id=28513867&tree=Try&full=1#error0

In the real world this would probably break Firefox completely. I pushed another cset [2] that just adds Math.random() and that seems to be a little more collision resistant. The ideas posted here are better though.

[1] https://tbpl.mozilla.org/?tree=Try&rev=673debddf97d
[2] https://tbpl.mozilla.org/?tree=Try&rev=c136704aa149
Attachment #811520 - Flags: review?(gavin.sharp)
Attachment #811520 - Flags: review?(dolske)
Comment on attachment 811520 [details] [diff] [review]
Avoid tab panel ID collisions by using a monotonic counter

THREE YEARS AND MOZILLA HAS STILL NOT FIX......oh. yay! \o/
Attachment #811520 - Flags: review?(gavin.sharp)
Attachment #811520 - Flags: review?(dolske)
Attachment #811520 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e56505c62aba
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
(In reply to Frank Yan from comment #3)
> Justin, does the unique ID have to be unique across all windows or just one
> window? (I'm guessing the latter.)

When SeaMonkey first ported bug 179656 in bug 105885 comment 108 we didn't bother with Date.now() instead we just used a per-tabbrowser incrementing counter. Nobody seems to have complained.
Blocks: MoveTabs
Version: Trunk → 1.5.0.x Branch
Comment on attachment 811520 [details] [diff] [review]
Avoid tab panel ID collisions by using a monotonic counter

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 179656
User impact if declined: Browser window might in very rare occasions get into an unusable state.
Testing completed (on m-c, etc.): Landed on m-c.
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None.

I think this would be great if we could uplift it as far as possible. It's a safe fix that eliminates a possible issue where esp. on Window we could render a whole browser window unusable.
Attachment #811520 - Flags: approval-mozilla-beta?
Attachment #811520 - Flags: approval-mozilla-aurora?
Comment on attachment 811520 [details] [diff] [review]
Avoid tab panel ID collisions by using a monotonic counter

Doesn't meet the criteria for branches past Aurora. Longstanding issue without new urgency.
Attachment #811520 - Flags: approval-mozilla-beta?
Attachment #811520 - Flags: approval-mozilla-beta-
Attachment #811520 - Flags: approval-mozilla-aurora?
Attachment #811520 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Could you please give some guidance for QA to verify this? Thanks!
Flags: needinfo?(ttaubert)
QA Contact: manuela.muntean
I don't think there is much you can do to verify this manually. I could reproduce bug 903834 quite regularly on my machine but with the patch applied I couldn't anymore. The intermittent failures from bug 903834 also don't occur anymore. Sorry, that's a rather technical issue.
Flags: needinfo?(ttaubert)
Marking this verified as fixed, based on comment 19. Thanks Tim!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.