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

Status: VERIFIED FIXED

(Firefox :: General, defect)

Comment 1

[Justin Dolske [:Dolske]]

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

[Rob Arnold [:robarnold]]

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

[Frank Yan (:fryn)]

Justin, does the unique ID have to be unique across all windows or just one window? (I'm guessing the latter.)

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 5

[Justin Dolske [:Dolske]]

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

[Frank Yan (:fryn)]

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

Comment 7

[Justin Dolske [:Dolske]]

nsIDOMWindowUtils.outerWindowID is what I'm thinking of.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Avoid tab panel ID collisions by using a monotonic counter

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=a8263e91626c

Comment 11

[Justin Dolske [: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/

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Thanks!

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

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e56505c62aba

Comment 14

[[e-mail]]

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 16

[Alex Keybl [:akeybl]]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/75e1f0a7290c

Comment 18

[Manuela Muntean [Away]]

Could you please give some guidance for QA to verify this? Thanks!

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 20

[Manuela Muntean [Away]]

Marking this verified as fixed, based on comment 19. Thanks Tim!