New issue
Advanced search Search tips

Issue 695198 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

ExtensionWebRequestEventRouter posts work to IO thread via WeakPtr but is itself a Singleton

Project Member Reported by w...@chromium.org, Feb 22 2017

Issue description

ExtensionWebRequestEventRouter uses SupportsWeakPtr to dispense WeakPtrs to itself to Bind() to tasks to run on the IO thread.

However, EWRER is itself a Singleton, so will be torn-down on the main (UI) thread, not the IO thread, so this arrangement doesn't make sense.
 
Cc: rdevlin....@chromium.org
Owner: karandeepb@chromium.org
Status: Assigned (was: Untriaged)
Karan's been poking around the webRequest API lately, over to him.

Since the ExtensionWebRequestEventRouter, it seems like it would make more sense to just make it leaky and use base::Unretained.

Comment 2 by w...@chromium.org, Feb 24 2017

FWIW that is what I did for the NaClBrowser singleton.
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c75f6f0f5286abfacf96b9ef7ac088a9692ed897

commit c75f6f0f5286abfacf96b9ef7ac088a9692ed897
Author: karandeepb <karandeepb@chromium.org>
Date: Wed Mar 01 03:01:08 2017

Extensions: Make ExtensionWebRequestEventRouter leaky.

ExtensionWebRequestEventRouter is a Singleton primarily used on the IO thread.
However, being a Singleton it is destroyed on the main/UI thread. This is
potentially not thread-safe.

To fix, make ExtensionWebRequestEventRouter a leaky singleton. This also removes
the need for ExtensionWebRequestEventRouter to support weak pointers (which
didn't add any safety in the first place, since invalidating weak pointers on
one thread while accessing them on another is not thread-safe).

BUG= 695198 

Review-Url: https://codereview.chromium.org/2721793002
Cr-Commit-Position: refs/heads/master@{#453806}

[modify] https://crrev.com/c75f6f0f5286abfacf96b9ef7ac088a9692ed897/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/c75f6f0f5286abfacf96b9ef7ac088a9692ed897/extensions/browser/api/web_request/web_request_api.h

Status: Fixed (was: Assigned)

Sign in to add a comment