RESOLVED FIXED 103259
Numeric identifiers of events are not guaranteed to be unique
https://bugs.webkit.org/show_bug.cgi?id=103259
Summary Numeric identifiers of events are not guaranteed to be unique
Cosmin Truta
Reported 2012-11-26 08:28:36 PST
The results of setTimeout, setInterval and navigator.geolocation.watchPosition are positive integer values extracted from a simple circular sequential number generator, whose uniqueness can be guaranteed for no more than 2^31 calls to any of these functions. In order to provide this guarantee beyond this limit, we repeatedly ask for the next sequential id until we get one that's not used already. With this solution we fix a FIXME note.
Attachments
Patch (8.94 KB, patch)
2012-11-26 09:10 PST, Cosmin Truta
no flags
Patch (8.98 KB, patch)
2012-11-29 15:46 PST, Cosmin Truta
darin: review-
darin: commit-queue-
Patch (8.75 KB, patch)
2012-11-29 19:24 PST, Cosmin Truta
ap: review+
ap: commit-queue-
Patch (8.71 KB, patch)
2013-02-14 12:53 PST, Cosmin Truta
no flags
Cosmin Truta
Comment 1 2012-11-26 09:10:46 PST
Cosmin Truta
Comment 2 2012-11-26 09:13:43 PST
+Alexey I addressed a FIXME note which I had originally added at his request.
Alexey Proskuryakov
Comment 3 2012-11-27 11:36:10 PST
Comment on attachment 176014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176014&action=review > Source/WebCore/Modules/geolocation/Geolocation.cpp:183 > + bool result = m_idToNotifierMap.add(id, notifier.get()).isNewEntry; Won't this add() call overwrite an entry in m_idToNotifierMap on collision?
Cosmin Truta
Comment 4 2012-11-27 19:48:08 PST
(In reply to comment #3) > Won't this add() call overwrite an entry in m_idToNotifierMap on collision? The original call (m_timeouts.set) used to. The new one (m_timeouts.add) does not. That is, essentially, the difference between HashMap::set and HashMap::add.
Cosmin Truta
Comment 5 2012-11-27 19:51:47 PST
(In reply to comment #4) > The original call (m_timeouts.set) used to. The new one (m_timeouts.add) does not. Sorry, I meant to refer to m_idToNotifierMap. In fact, the comment above applies both to m_idToNotifierMap and m_timeouts.
Alexey Proskuryakov
Comment 6 2012-11-27 20:03:52 PST
> That is, essentially, the difference between HashMap::set and HashMap::add. Why does add() return "isNewEntry" if it doesn't overwrite?
Cosmin Truta
Comment 7 2012-11-27 20:31:20 PST
(In reply to comment #6) > Why does add() return "isNewEntry" if it doesn't overwrite? Both HashMap::set and HashMap::add return an object of type HashTableType::AddResult, which is an (iterator, isNewEntry) tuple. The iterator member points to the newly-added or already-existing object (depending on what gets called); the isNewEntry member is true if a new object has just been set or added, and false in case of replacement (for set) or failure (for add). See the difference in the implementation of HashMap::set and HashMap::add. They both call HashMap::inlineAdd; but the difference is that HashMap::add does nothing on failure, while HashMap::set does the following: if (!result.isNewEntry) { // The inlineAdd call above found an existing hash table entry; we need to set the mapped value. MappedTraits::store(mapped, result.iterator->second); } In my patch, by replacing HashMap::set with HashMap::add, I no longer execute the above code, leaving the original key/value pair intact.
Cosmin Truta
Comment 8 2012-11-29 15:46:29 PST
Created attachment 176838 [details] Patch After discussing offline with Alexey about various alternative names for the "result" variable, I eventually opted for "addSucceeded". I found out this is a common naming pattern in WebKit.
Darin Adler
Comment 9 2012-11-29 17:10:48 PST
Comment on attachment 176838 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176838&action=review review- because of the strange RefPtr local variable thing; otherwise looks OK > Source/WebCore/Modules/geolocation/Geolocation.cpp:186 > + bool addSucceeded = m_idToNotifierMap.add(id, notifier.get()).isNewEntry; > + if (addSucceeded) > + m_notifierToIdMap.set(notifier.release(), id); > + return addSucceeded; I would write it this way: if (!m_idToNotifierMap.add(id, notifier.get()).isNewEntry) return false; m_notifierToIdMap.set(notifier.release(), id); return true; > Source/WebCore/Modules/geolocation/Geolocation.cpp:321 > + RefPtr<GeoNotifier> passNotifier = notifier; > + success = m_watchers.add(watchID, passNotifier.release()); This should just be: success = m_watches.add(watchID, notifier); There is no need for the passNotifier local variable.
Cosmin Truta
Comment 10 2012-11-29 19:24:52 PST
Created attachment 176889 [details] Patch (In reply to comment #9) Done; and I simplified it further, by removing the "success" variable. In addition, I made the "unlikely" comment accessible not only to the reader, but also to the compiler.
Cosmin Truta
Comment 11 2013-02-13 13:50:41 PST
I rerun the tests with my patch on today's master, and they are still passing. May I please have a review?
Yong Li
Comment 12 2013-02-13 14:16:28 PST
+Darin
Alexey Proskuryakov
Comment 13 2013-02-14 11:56:15 PST
Comment on attachment 176889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176889&action=review > Source/WebCore/Modules/geolocation/Geolocation.cpp:318 > + } while (UNLIKELY(!m_watchers.add(watchID, notifier))); Please don't put this UNLIKELY here. It's not hot code, and there is no need to add visual noise to it.
Cosmin Truta
Comment 14 2013-02-14 12:53:16 PST
Created attachment 188409 [details] Patch Thank you for the review. I removed UNLIKELY as you requested. Could you please cq+ the updated patch for me?
WebKit Review Bot
Comment 15 2013-02-14 13:51:00 PST
Comment on attachment 188409 [details] Patch Clearing flags on attachment: 188409 Committed r142909: <http://trac.webkit.org/changeset/142909>
WebKit Review Bot
Comment 16 2013-02-14 13:51:04 PST
All reviewed patches have been landed. Closing bug.
Lucas Forschler
Comment 17 2019-02-06 09:18:42 PST
Mass move bugs into the DOM component.
Note You need to log in before you can comment on or make changes to this bug.