Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2303113002:
Fix semantics of ExtensionWebRequestEventRouter::EventListeners. (Closed)
DescriptionFix 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
Committed: https://crrev.com/7cdfec136b896222345c88f56e89a2d40d53e49c
Cr-Commit-Position: refs/heads/master@{#417398}
Patch Set 1 #Patch Set 2 : clang format. #Patch Set 3 : Rebase. #Patch Set 4 : Compile error on Linux. #
Total comments: 20
Patch Set 5 : Comments from rdevlin. #
Total comments: 16
Patch Set 6 : Comments from rdevlin.cronin. #
Total comments: 4
Patch Set 7 : Comments from rdevlin. #
Messages
Total messages: 27 (15 generated)
Description was changed from ========== Fixing webrequestapi issues. BUG= ========== to ========== Refactor and crash fixes to WebRequestAPI. 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 a clear semantics for equality. The new code makes several changes: 1) Create the class EventListener::Identifier. 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::Identifier 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, so all comparisons use EventListener::Identifier. This was the root cause of the leak from Issue 643025. BUG=643025, 641958 ==========
Description was changed from ========== Refactor and crash fixes to WebRequestAPI. 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 a clear semantics for equality. The new code makes several changes: 1) Create the class EventListener::Identifier. 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::Identifier 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, so all comparisons use EventListener::Identifier. This was the root cause of the leak from Issue 643025. BUG=643025, 641958 ========== to ========== 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 a clear semantics for equality. The new code makes several changes: 1) Create the class EventListener::Identifier. 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::Identifier 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, so all comparisons use EventListener::Identifier. This was the root cause of the leak from Issue 643025. BUG=643025, 641958 ==========
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
erikchen@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin: Please review.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/09/02 02:14:21, erikchen wrote: > rdevlin: Please review. haven't taken a close look yet, but seems like there's some red bots here?
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/02 04:00:10, Devlin wrote: > On 2016/09/02 02:14:21, erikchen wrote: > > rdevlin: Please review. > > haven't taken a close look yet, but seems like there's some red bots here? PTAL
https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:984: EventListener* listener = FindEventListener(id); This makes me a bit sad, because it means we have to look up the listener again (and this code needs to be as fast as possible). What if we instead keep GetMatchingListeners returning a std::vector<EventListener*>, as it does now, but then construct the ids in this function for when we dispatch them? So just adding: std::unique_ptr<ListenerIdentifiers> listener_ids(new ListenerIdentifiers()); // Note std::unique_ptr for reason below listener_ids->reserve(listeners.size()); for (listener : listeners) { listener_ids->push_back(listener->id()); ... } Would that work? https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1004: AsWeakPtr(), browser_context, listeners)); Optimization: Shame about the copy this causes on listeners, even if it is cheaper. Why not pass ownership? It's pretty trivial if we can do the comment on 984, but even if not, it looks like we could pass ownership of |listeners| to DispatchEvent() and then to this closure so that we don't have to perform any copies. We just need to return a unique_ptr from GetMatchingListeners(). WDYT? https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1023: DCHECK_GT(listener_ids.size(), 0UL); nit: DCHECK(!listener_ids.empty()) https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1040: for (const std::unique_ptr<EventListener>& event_listener : Worth making a FindEventListenerInSet() helper function that can be used here, line 1048, and in FindEventListener()? https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.h (right): https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:331: // An EventListener is uniquely defined by five properties. This isn't strictly true, right? We potentially have multiple listeners with the same id. At least, we don't enforce we don't, and you have a test that does... And the more I look at it, the more that scares me. *Can* we enforce that there's only one listener with a given id? https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:338: : browser_context(browser_context), nit: these methods should probably be defined in the .cc. https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:345: bool LooselyMatches(const Identifier& that) const { Is there a time we don't want to do this? i.e., why not just have this be the operator== implementation? https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:386: friend class WebRequestAPI; Why does the WebRequestAPI need to be able to copy these?
rdevlin: PTAL https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:984: EventListener* listener = FindEventListener(id); On 2016/09/07 19:18:00, Devlin wrote: > This makes me a bit sad, because it means we have to look up the listener again > (and this code needs to be as fast as possible). What if we instead keep > GetMatchingListeners returning a std::vector<EventListener*>, as it does now, > but then construct the ids in this function for when we dispatch them? So just > adding: > std::unique_ptr<ListenerIdentifiers> listener_ids(new ListenerIdentifiers()); > // Note std::unique_ptr for reason below > listener_ids->reserve(listeners.size()); > for (listener : listeners) { > listener_ids->push_back(listener->id()); > ... > } > > Would that work? yup, done. https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1004: AsWeakPtr(), browser_context, listeners)); On 2016/09/07 19:18:00, Devlin wrote: > Optimization: Shame about the copy this causes on listeners, even if it is > cheaper. Why not pass ownership? It's pretty trivial if we can do the comment > on 984, but even if not, it looks like we could pass ownership of |listeners| to > DispatchEvent() and then to this closure so that we don't have to perform any > copies. We just need to return a unique_ptr from GetMatchingListeners(). WDYT? Done. https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1023: DCHECK_GT(listener_ids.size(), 0UL); On 2016/09/07 19:18:00, Devlin wrote: > nit: DCHECK(!listener_ids.empty()) Done. https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1040: for (const std::unique_ptr<EventListener>& event_listener : On 2016/09/07 19:18:00, Devlin wrote: > Worth making a FindEventListenerInSet() helper function that can be used here, > line 1048, and in FindEventListener()? yes, done. https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.h (right): https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:331: // An EventListener is uniquely defined by five properties. On 2016/09/07 19:18:00, Devlin wrote: > This isn't strictly true, right? We potentially have multiple listeners with > the same id. At least, we don't enforce we don't, and you have a test that > does... > > And the more I look at it, the more that scares me. *Can* we enforce that > there's only one listener with a given id? Two responses: 1) This comment was based on my interpretation of the code in ExtensionWebRequestEventRouter::AddEventListener: """ if (base::ContainsKey(listeners_[browser_context][event_name], listener)) { // This is likely an abuse of the API by a malicious extension. return false; } 2) I don't know anything about the semantics of WebRequest Event Listeners. If it should be possible to have multiple listeners with the same "id", then the previous code was wrong and should be fixed in a followup CL. https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:338: : browser_context(browser_context), On 2016/09/07 19:18:00, Devlin wrote: > nit: these methods should probably be defined in the .cc. Done. https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:345: bool LooselyMatches(const Identifier& that) const { On 2016/09/07 19:18:00, Devlin wrote: > Is there a time we don't want to do this? i.e., why not just have this be the > operator== implementation? We can't use operator== everywhere because we aren't guaranteed to have the right embedder_process_id at certain points in time (see the reverted CL). We can't use LooselyMatches since it's possible for multiple IDs to "loosely match" when a web_view_instance_id exists. https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:386: friend class WebRequestAPI; On 2016/09/07 19:18:00, Devlin wrote: > Why does the WebRequestAPI need to be able to copy these? Removed.
https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.h (right): https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:331: // An EventListener is uniquely defined by five properties. On 2016/09/07 22:07:43, erikchen wrote: > On 2016/09/07 19:18:00, Devlin wrote: > > This isn't strictly true, right? We potentially have multiple listeners with > > the same id. At least, we don't enforce we don't, and you have a test that > > does... > > > > And the more I look at it, the more that scares me. *Can* we enforce that > > there's only one listener with a given id? > > Two responses: > 1) This comment was based on my interpretation of the code in > ExtensionWebRequestEventRouter::AddEventListener: > """ > if (base::ContainsKey(listeners_[browser_context][event_name], listener)) { > // This is likely an abuse of the API by a malicious extension. > return false; > } Yep, that and others are why I was scared. :) > 2) I don't know anything about the semantics of WebRequest Event Listeners. If > it should be possible to have multiple listeners with the same "id", then the > previous code was wrong and should be fixed in a followup CL. Can we add a TODO and avoid using multiple listeners with the same id in new tests? https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:345: bool LooselyMatches(const Identifier& that) const { On 2016/09/07 22:07:43, erikchen wrote: > On 2016/09/07 19:18:00, Devlin wrote: > > Is there a time we don't want to do this? i.e., why not just have this be the > > operator== implementation? > > We can't use operator== everywhere because we aren't guaranteed to have the > right embedder_process_id at certain points in time (see the reverted CL). We > can't use LooselyMatches since it's possible for multiple IDs to "loosely match" > when a web_view_instance_id exists. Ah, I see. This is annoying. Can we add another TODO to remove this, so that the world is slightly more sane? :) https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (left): https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1208: // Debugging https://crbug.com/589735 Hmm... so this does fix the bug of the release assert, since we no longer use a std::set here. But it also means we could be removing arbitrarily many listeners, which is probably not what we want. Is that right? (That should be okay to address in a followup) https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1113: if (listener->id.web_view_instance_id) { nit: s/listener->id.web_view_instance_id/web_view_instance_id https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1118: if (FindEventListener(id) != nullptr) { Now that we search by id, we can move this before constructing the listener, right? https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1142: ExtensionWebRequestEventRouter::FindEventListenerInContainer( nit: put this in the anonymous namespace instead? https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1166: blocked_request_id, NULL); nit: nullptr https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.h (right): https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:332: struct Identifier { nit: s/Identifier/ID? https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:367: using ListenerPointers = std::vector<EventListener*>; s/ListenerPointers/WeakListeners?
https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.h (right): https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:331: // An EventListener is uniquely defined by five properties. On 2016/09/08 17:33:12, Devlin wrote: > On 2016/09/07 22:07:43, erikchen wrote: > > On 2016/09/07 19:18:00, Devlin wrote: > > > This isn't strictly true, right? We potentially have multiple listeners > with > > > the same id. At least, we don't enforce we don't, and you have a test that > > > does... > > > > > > And the more I look at it, the more that scares me. *Can* we enforce that > > > there's only one listener with a given id? > > > > Two responses: > > 1) This comment was based on my interpretation of the code in > > ExtensionWebRequestEventRouter::AddEventListener: > > """ > > if (base::ContainsKey(listeners_[browser_context][event_name], listener)) { > > // This is likely an abuse of the API by a malicious extension. > > return false; > > } > > Yep, that and others are why I was scared. :) > > > 2) I don't know anything about the semantics of WebRequest Event Listeners. If > > it should be possible to have multiple listeners with the same "id", then the > > previous code was wrong and should be fixed in a followup CL. > > Can we add a TODO and avoid using multiple listeners with the same id in new > tests? I added a long TODO & comment. We still need to use the "same id" for non-strict removal of event listeners (embedder_process_id = 0). Note that that test was added in https://codereview.chromium.org/2121873002. I'd be willing to believe that for non web view event listeners, the tuple of (browser_context, extension_id, sub_event_name) already uniquely specifies an extension, in which case the test would be incorrect and misleading, but that's not immediately clear to me. https://codereview.chromium.org/2303113002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:345: bool LooselyMatches(const Identifier& that) const { On 2016/09/08 17:33:12, Devlin wrote: > On 2016/09/07 22:07:43, erikchen wrote: > > On 2016/09/07 19:18:00, Devlin wrote: > > > Is there a time we don't want to do this? i.e., why not just have this be > the > > > operator== implementation? > > > > We can't use operator== everywhere because we aren't guaranteed to have the > > right embedder_process_id at certain points in time (see the reverted CL). We > > can't use LooselyMatches since it's possible for multiple IDs to "loosely > match" > > when a web_view_instance_id exists. > > Ah, I see. This is annoying. Can we add another TODO to remove this, so that > the world is slightly more sane? :) Added a TODO. https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (left): https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1208: // Debugging https://crbug.com/589735 On 2016/09/08 17:33:12, Devlin wrote: > Hmm... so this does fix the bug of the release assert, since we no longer use a > std::set here. But it also means we could be removing arbitrarily many > listeners, which is probably not what we want. Is that right? No. My CL has the same behavior as the previous code, which is the removal of the first event_listener that matches. Note the early return. I added a |strict| parameter to the function to make the behavior more clear. So this fixes the issue with undefined std::set sorting logic, without tackling the [potentially larger] issue that non WebView event listeners are sometimes poorly specified by the API [the API doesn't always include embedder_process_id - the built in assumption is that only a single embedder_process_id can exist for each browser_context, extension_id, sub_event_name tuple]. https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1113: if (listener->id.web_view_instance_id) { On 2016/09/08 17:33:12, Devlin wrote: > nit: s/listener->id.web_view_instance_id/web_view_instance_id Done. https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1118: if (FindEventListener(id) != nullptr) { On 2016/09/08 17:33:12, Devlin wrote: > Now that we search by id, we can move this before constructing the listener, > right? Done. https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1142: ExtensionWebRequestEventRouter::FindEventListenerInContainer( On 2016/09/08 17:33:12, Devlin wrote: > nit: put this in the anonymous namespace instead? Can't do that, as this uses private members of ExtensionWebRequestEventRouter. https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1166: blocked_request_id, NULL); On 2016/09/08 17:33:12, Devlin wrote: > nit: nullptr Done. https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.h (right): https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:332: struct Identifier { On 2016/09/08 17:33:12, Devlin wrote: > nit: s/Identifier/ID? Done. https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.h:367: using ListenerPointers = std::vector<EventListener*>; On 2016/09/08 17:33:12, Devlin wrote: > s/ListenerPointers/WeakListeners? I used RawListeners, since weak pointers have different semantics from raw pointers.
lgtm. Please also update the CL description since it's slightly out of date (e.g., EventListener::Identifier changed) https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (left): https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1208: // Debugging https://crbug.com/589735 On 2016/09/08 18:34:42, erikchen wrote: > On 2016/09/08 17:33:12, Devlin wrote: > > Hmm... so this does fix the bug of the release assert, since we no longer use > a > > std::set here. But it also means we could be removing arbitrarily many > > listeners, which is probably not what we want. Is that right? > No. My CL has the same behavior as the previous code, which is the removal of > the first event_listener that matches. Note the early return. > > I added a |strict| parameter to the function to make the behavior more clear. > > So this fixes the issue with undefined std::set sorting logic, without tackling > the [potentially larger] issue that non WebView event listeners are sometimes > poorly specified by the API [the API doesn't always include embedder_process_id > - the built in assumption is that only a single embedder_process_id can exist > for each browser_context, extension_id, sub_event_name tuple]. Right, that's what I was saying. This fixes the release assert that we would previously hit through virtue of no longer asserting there is one and only one matching listener, and this removes the first listener with the matching id, but we still could have multiple listeners (which potentially could lead to leaks). I think we're on the same page here. https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2303113002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1142: ExtensionWebRequestEventRouter::FindEventListenerInContainer( On 2016/09/08 18:34:42, erikchen wrote: > On 2016/09/08 17:33:12, Devlin wrote: > > nit: put this in the anonymous namespace instead? > > Can't do that, as this uses private members of ExtensionWebRequestEventRouter. Oh, sneaky. Forgot that event listener id itself was a private member. https://codereview.chromium.org/2303113002/diff/100001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2303113002/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:367: false)); add /* not strict */ (or // not strict) here https://codereview.chromium.org/2303113002/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:1204: RemoveEventListener(listener_id, true); // strict
Description was changed from ========== 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 a clear semantics for equality. The new code makes several changes: 1) Create the class EventListener::Identifier. 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::Identifier 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, so all comparisons use EventListener::Identifier. This was the root cause of the leak from Issue 643025. BUG=643025, 641958 ========== to ========== 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 ==========
https://codereview.chromium.org/2303113002/diff/100001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2303113002/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:367: false)); On 2016/09/08 19:53:32, Devlin wrote: > add /* not strict */ (or // not strict) here Done. https://codereview.chromium.org/2303113002/diff/100001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:1204: RemoveEventListener(listener_id, true); On 2016/09/08 19:53:32, Devlin wrote: > // strict Done.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2303113002/#ps120001 (title: "Comments from rdevlin.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/7cdfec136b896222345c88f56e89a2d40d53e49c Cr-Commit-Position: refs/heads/master@{#417398} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7cdfec136b896222345c88f56e89a2d40d53e49c Cr-Commit-Position: refs/heads/master@{#417398} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
