WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(8.98 KB, patch)
2012-11-29 15:46 PST
,
Cosmin Truta
darin
: review-
darin
: commit-queue-
Details
Formatted Diff
Diff
Patch
(8.75 KB, patch)
2012-11-29 19:24 PST
,
Cosmin Truta
ap
: review+
ap
: commit-queue-
Details
Formatted Diff
Diff
Patch
(8.71 KB, patch)
2013-02-14 12:53 PST
,
Cosmin Truta
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Cosmin Truta
Comment 1
2012-11-26 09:10:46 PST
Created
attachment 176014
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug