Closed
Bug 586153
Opened 14 years ago
Closed 11 years ago
unique panel ID shouldn't use Date.now()
Categories
(Firefox :: General, defect)
Tracking
()
VERIFIED
FIXED
Firefox 27
People
(Reporter: Dolske, Assigned: ttaubert)
References
Details
(Keywords: verifyme)
Attachments
(1 file)
(deleted),
patch
|
Dolske
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•14 years ago
|
||
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.
Comment 2•14 years ago
|
||
Dates are the wrong source of unique IDs on all platforms for a number of reasons - a monotonic counter is the way to go.
Comment 3•14 years ago
|
||
Justin, does the unique ID have to be unique across all windows or just one window? (I'm guessing the latter.)
Comment 4•14 years ago
|
||
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.
Reporter | ||
Comment 5•14 years ago
|
||
Yeah, seems safest to keep them globally unique. You could probably just append a tab counter to the window ID to do so?
Comment 6•14 years ago
|
||
(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?
Reporter | ||
Comment 7•14 years ago
|
||
nsIDOMWindowUtils.outerWindowID is what I'm thinking of.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 8•11 years ago
|
||
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
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #811520 -
Flags: review?(gavin.sharp)
Attachment #811520 -
Flags: review?(dolske)
Assignee | ||
Comment 10•11 years ago
|
||
Reporter | ||
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Comment 14•11 years ago
|
||
(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.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 15•11 years ago
|
||
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 16•11 years ago
|
||
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+
Comment 17•11 years ago
|
||
Comment 18•11 years ago
|
||
Could you please give some guidance for QA to verify this? Thanks!
Flags: needinfo?(ttaubert)
Updated•11 years ago
|
QA Contact: manuela.muntean
Assignee | ||
Comment 19•11 years ago
|
||
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)
Comment 20•11 years ago
|
||
Marking this verified as fixed, based on comment 19. Thanks Tim!
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•