[Merge to 2840] Fix semantics of ExtensionWebRequestEventRouter::EventListeners.

[Merge to 2840] Fix semantics of ExtensionWebRequestEventRouter::EventListeners.

> There are two types of EventListeners: those with web_view_instance_id and those
> without. EventListeners with a web_view_instance_id need to always consider
> embedder_process_id for equality comparisons. EventListeners without a
> web_view_instance_id should sometimes ignore embedder_process_id (when being
> removed) for equality comparisons.
>
> The previous code had two major issues:
>   1) EventListeners were stored in a std::set. std:set requires a stable sort
>   operator. There is no way to create a stable sort operator that sometimes
>   ignores embedder_process_id.
>
>   Aside: std::set is almost never the right container to use from a performance
>   perspective. In this case, the code frequently needs to iterate through the
>   entire container away, so I've changed the container to be a std::vector (we
>   expect the total number of elements to be small). If we find performance
>   issues, we should switch to std::unordered_set.
>
>   2) EventListeners need to be messaged asynchronously, but it's possible that
>   the EventListener is no longer registered by the time the callback comes
>   around. The previous code would make a copy of the EventListener, and then try
>   to "find" the original copy in the callback. Once again, this requires clear
>   semantics for equality.
>
> The new code makes several changes:
>   1) Create the class EventListener::ID. As its name implies, this uniquely
>   identifies an EventListener.
>
>   2) Disallow copy and assignment of EventListeners. The only use case is async
>   identity checking, which should use EventListener::ID instead.
>
>   3) This cleanup means EventListener::blocked_requests no longer has to be
>   mutable. (A clear sign that something was wrong with the previous scheme).
>
> The most important result is that it is no longer possible to have identity
> confusion between EventListeners, as all comparisons use EventListener::ID. This
> was the root cause of the leak from Issue 643025.
>
> BUG=643025, 641958
>
> Review-Url: https://codereview.chromium.org/2303113002
> Cr-Commit-Position: refs/heads/master@{#417398}
> (cherry picked from commit 7cdfec136b896222345c88f56e89a2d40d53e49c)

Committed: https://chromium.googlesource.com/chromium/src/+/f5f0196947ef79ab06a121329945ed3141c9b095

[erikchen, 2016-09-15 18:32:40]

Description was changed from

==========
[Merge to 2840] Fix semantics of ExtensionWebRequestEventRouter::EventListeners.

> There are two types of EventListeners: those with web_view_instance_id and
those
> without. EventListeners with a web_view_instance_id need to always consider
> embedder_process_id for equality comparisons. EventListeners without a
> web_view_instance_id should sometimes ignore embedder_process_id (when being
> removed) for equality comparisons.
>
> The previous code had two major issues:
>   1) EventListeners were stored in a std::set. std:set requires a stable sort
>   operator. There is no way to create a stable sort operator that sometimes
>   ignores embedder_process_id.
>
>   Aside: std::set is almost never the right container to use from a
performance
>   perspective. In this case, the code frequently needs to iterate through the
>   entire container away, so I've changed the container to be a std::vector (we
>   expect the total number of elements to be small). If we find performance
>   issues, we should switch to std::unordered_set.
>
>   2) EventListeners need to be messaged asynchronously, but it's possible that
>   the EventListener is no longer registered by the time the callback comes
>   around. The previous code would make a copy of the EventListener, and then
try
>   to "find" the original copy in the callback. Once again, this requires clear
>   semantics for equality.
>
> The new code makes several changes:
>   1) Create the class EventListener::ID. As its name implies, this uniquely
>   identifies an EventListener.
>
>   2) Disallow copy and assignment of EventListeners. The only use case is
async
>   identity checking, which should use EventListener::ID instead.
>
>   3) This cleanup means EventListener::blocked_requests no longer has to be
>   mutable. (A clear sign that something was wrong with the previous scheme).
>
> The most important result is that it is no longer possible to have identity
> confusion between EventListeners, as all comparisons use EventListener::ID.
This
> was the root cause of the leak from Issue 643025.
>
> BUG=643025, 641958
>
> Review-Url: https://codereview.chromium.org/2303113002
> Cr-Commit-Position: refs/heads/master@{#417398}
> (cherry picked from commit 7cdfec136b896222345c88f56e89a2d40d53e49c)
==========

to

==========
[Merge to 2840] Fix semantics of ExtensionWebRequestEventRouter::EventListeners.

> There are two types of EventListeners: those with web_view_instance_id and
those
> without. EventListeners with a web_view_instance_id need to always consider
> embedder_process_id for equality comparisons. EventListeners without a
> web_view_instance_id should sometimes ignore embedder_process_id (when being
> removed) for equality comparisons.
>
> The previous code had two major issues:
>   1) EventListeners were stored in a std::set. std:set requires a stable sort
>   operator. There is no way to create a stable sort operator that sometimes
>   ignores embedder_process_id.
>
>   Aside: std::set is almost never the right container to use from a
performance
>   perspective. In this case, the code frequently needs to iterate through the
>   entire container away, so I've changed the container to be a std::vector (we
>   expect the total number of elements to be small). If we find performance
>   issues, we should switch to std::unordered_set.
>
>   2) EventListeners need to be messaged asynchronously, but it's possible that
>   the EventListener is no longer registered by the time the callback comes
>   around. The previous code would make a copy of the EventListener, and then
try
>   to "find" the original copy in the callback. Once again, this requires clear
>   semantics for equality.
>
> The new code makes several changes:
>   1) Create the class EventListener::ID. As its name implies, this uniquely
>   identifies an EventListener.
>
>   2) Disallow copy and assignment of EventListeners. The only use case is
async
>   identity checking, which should use EventListener::ID instead.
>
>   3) This cleanup means EventListener::blocked_requests no longer has to be
>   mutable. (A clear sign that something was wrong with the previous scheme).
>
> The most important result is that it is no longer possible to have identity
> confusion between EventListeners, as all comparisons use EventListener::ID.
This
> was the root cause of the leak from Issue 643025.
>
> BUG=643025, 641958
>
> Review-Url: https://codereview.chromium.org/2303113002
> Cr-Commit-Position: refs/heads/master@{#417398}
> (cherry picked from commit 7cdfec136b896222345c88f56e89a2d40d53e49c)

Committed:
https://chromium.googlesource.com/chromium/src/+/f5f0196947ef79ab06a121329945...
==========

[erikchen, 2016-09-15 18:32:42]

Committed patchset #1 (id:1) manually as
f5f0196947ef79ab06a121329945ed3141c9b095.