Bug 103259 – Numeric identifiers of events are not guaranteed to be unique

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Cosmin Truta]

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.

[Cosmin Truta]

Created attachment 176014 [details]
Patch

[Cosmin Truta]

+Alexey
I addressed a FIXME note which I had originally added at his request.

[Alexey Proskuryakov]

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]

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

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

> That is, essentially, the difference between HashMap::set and HashMap::add.

Why does add() return "isNewEntry" if it doesn't overwrite?

[Cosmin Truta]

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

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 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]

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]

I rerun the tests with my patch on today's master, and they are still passing. May I please have a review?

[Yong Li]

+Darin

[Alexey Proskuryakov]

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]

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 on attachment 188409 [details]
Patch

Clearing flags on attachment: 188409

Committed r142909: <http://trac.webkit.org/changeset/142909>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass move bugs into the DOM component.