Ensure we don't load plugins on the IO thread

Ensure we don't load plugins on the IO thread.

I had to move the locks from PluginService to PluginList, so that a lock (which can block other threads) isn't held while loading the plugins.

BUG=17938
TEST=added asserts which crash if plugins loaded on IO thread, current UI tests exercise them

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=23501

[abarth-chromium, 2009-08-11 03:06:37]

This looks great except for the one question below.  Thanks.

http://codereview.chromium.org/164305/diff/27/34
File chrome/browser/renderer_host/buffered_resource_handler.cc (left):

http://codereview.chromium.org/164305/diff/27/34#oldcode121
Line 121: sniff_content_ = should_buffer_ = false;
Why did we remove this line?

[jam, 2009-08-11 04:08:16]

http://codereview.chromium.org/164305/diff/27/34
File chrome/browser/renderer_host/buffered_resource_handler.cc (left):

http://codereview.chromium.org/164305/diff/27/34#oldcode121
Line 121: sniff_content_ = should_buffer_ = false;
On 2009/08/11 03:06:37, abarth wrote:
> Why did we remove this line?


The first part wasn't needed because sniff_content_ is always set to false
before KeepBuffering returns false.  I also made a similar change to
should_buffer_ in my change.

[abarth-chromium, 2009-08-11 04:14:28]

Ok.  LGTM.

Adam


On Mon, Aug 10, 2009 at 9:08 PM, <jam@chromium.org> wrote:
>
> http://codereview.chromium.org/164305/diff/27/34
> File chrome/browser/renderer_host/buffered_resource_handler.cc (left):
>
> http://codereview.chromium.org/164305/diff/27/34#oldcode121
> Line 121: sniff_content_ =3D should_buffer_ =3D false;
> On 2009/08/11 03:06:37, abarth wrote:
>>
>> Why did we remove this line?
>
>
> The first part wasn't needed because sniff_content_ is always set to
> false before KeepBuffering returns false. =A0I also made a similar change
> to should_buffer_ in my change.
>
> http://codereview.chromium.org/164305
>

[jam, 2009-08-12 00:44:46]

Please have a look at the latest version, to fix some crashes I added an
OnRequestClosed callback to ResourceHandler so that it knows that the URLRequest
object has gone away.

[jam, 2009-08-12 01:42:40]

Actually, hold on from looking at this.  I just realized that the big locks
in PluginService still mean the IO thread will be blocked on the file
thread.  I'll take care of the locking.

On Tue, Aug 11, 2009 at 5:44 PM, <jam@chromium.org> wrote:

> Please have a look at the latest version, to fix some crashes I added an
> OnRequestClosed callback to ResourceHandler so that it knows that the
> URLRequest object has gone away.
>
>
> http://codereview.chromium.org/164305
>

[jam, 2009-08-12 03:00:17]

ok, this is now ready for a second look. I had to move the locks from
PluginService to PluginList so that a lock (which can block other threads)
isn't held while loading the plugins, which ended up touching a lot of code.
On Tue, Aug 11, 2009 at 6:42 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> Actually, hold on from looking at this.  I just realized that the big locks
> in PluginService still mean the IO thread will be blocked on the file
> thread.  I'll take care of the locking.
>
>
> On Tue, Aug 11, 2009 at 5:44 PM, <jam@chromium.org> wrote:
>
>> Please have a look at the latest version, to fix some crashes I added an
>> OnRequestClosed callback to ResourceHandler so that it knows that the
>> URLRequest object has gone away.
>>
>>
>> http://codereview.chromium.org/164305
>>
>
>

[abarth-chromium, 2009-08-12 03:52:05]

Ok.  I added a bunch of nits in PluginList.  Basically, you want to make it
obvious that every function is OK w.r.t. thread safety.  Every function should
have one of these four things:

1) An autolock.
2) A comment that it's declared static.
3) A DCHECK that we already hold the lock.
4) A DCHECK that it's always called on some specific thread.

There are a few more functions you should declare static so that the compiler
will force you to avoid the |this| pointer.

Once you do all that stuff, the rest looks fine to me.

[abarth-chromium, 2009-08-12 03:52:14]

http://codereview.chromium.org/164305/diff/63/66
File chrome/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/164305/diff/63/66#newcode399
Line 399: void BufferedResourceHandler::LoadPlugins() {
Should DCHECK which thread we're on now.

http://codereview.chromium.org/164305/diff/63/66#newcode406
Line 406: void BufferedResourceHandler::OnPluginsLoaded() {
DCHECK which thread we're on

http://codereview.chromium.org/164305/diff/63/69
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/164305/diff/63/69#newcode111
Line 111: virtual ~ExtraRequestInfo() { resource_handler->OnRequestClosed(); }
I presume you've checked that URLRequest::UserData has a virtual destructor.

http://codereview.chromium.org/164305/diff/63/71
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/164305/diff/63/71#newcode551
Line 551: IPC::Message* reply_msg) {
DCHECK which thread we're on

http://codereview.chromium.org/164305/diff/63/73
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/164305/diff/63/73#newcode156
Line 156: 
Can we move the code that runs unlocked to a static method to ensure that we
don't add any dependencies on |this| ?

http://codereview.chromium.org/164305/diff/63/73#newcode162
Line 162: GetPluginDirectories(&directories_to_scan);
Seems like we also want to DCHECK that we're on the FILE thread here.

http://codereview.chromium.org/164305/diff/63/73#newcode169
Line 169: }
No braces.

http://codereview.chromium.org/164305/diff/63/73#newcode173
Line 173: }
No braces.

http://codereview.chromium.org/164305/diff/63/73#newcode221
Line 221: WebPluginInfo* info) {
Is there a way to assert that we hold lock_ ?

http://codereview.chromium.org/164305/diff/63/73#newcode244
Line 244: WebPluginInfo* info) {
ditto

http://codereview.chromium.org/164305/diff/63/73#newcode262
Line 262: 
// static

http://codereview.chromium.org/164305/diff/63/73#newcode281
Line 281: 
// static

http://codereview.chromium.org/164305/diff/63/73#newcode300
Line 300: void PluginList::GetPlugins(bool refresh, std::vector<WebPluginInfo>*
plugins) {
DCHECK that we hold lock_

http://codereview.chromium.org/164305/diff/63/73#newcode310
Line 310: std::string* actual_mime_type) {
ditto

http://codereview.chromium.org/164305/diff/63/73#newcode331
Line 331: WebPluginInfo* info) {
ditto

http://codereview.chromium.org/164305/diff/63/74
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/164305/diff/63/74#newcode131
Line 131: // Load a specific plugin with full path.
static

http://codereview.chromium.org/164305/diff/63/74#newcode142
Line 142: // Load all plugins from a specific directory
static

http://codereview.chromium.org/164305/diff/63/74#newcode149
Line 149: // Load internal plugins.
static

http://codereview.chromium.org/164305/diff/63/76
File webkit/glue/plugins/plugin_list_linux.cc (right):

http://codereview.chromium.org/164305/diff/63/76#newcode14
Line 14: 
// static

http://codereview.chromium.org/164305/diff/63/76#newcode46
Line 46: 
// static

http://codereview.chromium.org/164305/diff/63/76#newcode69
Line 69: 
// static

http://codereview.chromium.org/164305/diff/63/78
File webkit/glue/plugins/plugin_list_mac.mm (right):

http://codereview.chromium.org/164305/diff/63/78#newcode47
Line 47: 
// static

http://codereview.chromium.org/164305/diff/63/78#newcode58
Line 58: 
// static

http://codereview.chromium.org/164305/diff/63/78#newcode69
Line 69: 
// static

http://codereview.chromium.org/164305/diff/63/78#newcode92
Line 92: 
// static

http://codereview.chromium.org/164305/diff/63/75
File webkit/glue/plugins/plugin_list_win.cc (right):

http://codereview.chromium.org/164305/diff/63/75#newcode250
Line 250: 
// static

http://codereview.chromium.org/164305/diff/63/75#newcode277
Line 277: 
// static

http://codereview.chromium.org/164305/diff/63/75#newcode304
Line 304: bool IsNewerVersion(const std::wstring& a, const std::wstring& b) {
either put this in an anonymous names space or declare it static.

http://codereview.chromium.org/164305/diff/63/75#newcode322
Line 322: 
DCHECK that we hold the lock

http://codereview.chromium.org/164305/diff/63/75#newcode380
Line 380: 
// static

[jam, 2009-08-13 00:01:28]

http://codereview.chromium.org/164305/diff/63/66
File chrome/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/164305/diff/63/66#newcode399
Line 399: void BufferedResourceHandler::LoadPlugins() {
On 2009/08/12 03:52:14, abarth wrote:
> Should DCHECK which thread we're on now.

I think this, and the one below, would be gratuitous to add.  The reason we have
PostTask is to execute some code on another thread, but going through all our
tasks or runnable methods and doing that wouldn't improve code quality.

http://codereview.chromium.org/164305/diff/63/69
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/164305/diff/63/69#newcode111
Line 111: virtual ~ExtraRequestInfo() { resource_handler->OnRequestClosed(); }
On 2009/08/12 03:52:14, abarth wrote:
> I presume you've checked that URLRequest::UserData has a virtual destructor.

yes, we already depend on that in this class (i.e. scoped ptrs).

http://codereview.chromium.org/164305/diff/63/71
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/164305/diff/63/71#newcode551
Line 551: IPC::Message* reply_msg) {
On 2009/08/12 03:52:14, abarth wrote:
> DCHECK which thread we're on

same as before, I think this doesn't improve code quality, as the code that
calls this function is a few lines above.

http://codereview.chromium.org/164305/diff/63/73
File webkit/glue/plugins/plugin_list.cc (right):

http://codereview.chromium.org/164305/diff/63/73#newcode156
Line 156: 
On 2009/08/12 03:52:14, abarth wrote:
> Can we move the code that runs unlocked to a static method to ensure that we
> don't add any dependencies on |this| ?

I prefer not to: I think this is a style issue, and I lean towards assuming that
the next developer modifying this function will be competent and will understand
the code that they're changing, and same for reviewers.  If they're not, they'll
still figure out a way to shoot themselves in the foot.

http://codereview.chromium.org/164305/diff/63/73#newcode162
Line 162: GetPluginDirectories(&directories_to_scan);
On 2009/08/12 03:52:14, abarth wrote:
> Seems like we also want to DCHECK that we're on the FILE thread here.

this code is in glue, which has no notion of threads.  also in testshell there's
no concept of file thread.

http://codereview.chromium.org/164305/diff/63/73#newcode169
Line 169: }
On 2009/08/12 03:52:14, abarth wrote:
> No braces.

I don't like unnecessary braces myself, but usually I prefer to leave the
existing style intact unless it violates the style guide.  In this case, it
doesn't.

http://codereview.chromium.org/164305/diff/63/73#newcode221
Line 221: WebPluginInfo* info) {
On 2009/08/12 03:52:14, abarth wrote:
> Is there a way to assert that we hold lock_ ?

there is, but the calling function wasn't locking, so I added an autolock.  good
catch.

http://codereview.chromium.org/164305/diff/63/73#newcode262
Line 262: 
On 2009/08/12 03:52:14, abarth wrote:
> // static

I think this is a style preference, i.e. it's not in the style guide.  I find it
gratuitous.

http://codereview.chromium.org/164305/diff/63/73#newcode300
Line 300: void PluginList::GetPlugins(bool refresh, std::vector<WebPluginInfo>*
plugins) {
On 2009/08/12 03:52:14, abarth wrote:
> DCHECK that we hold lock_

I was missing an autolock, added

http://codereview.chromium.org/164305/diff/63/74
File webkit/glue/plugins/plugin_list.h (right):

http://codereview.chromium.org/164305/diff/63/74#newcode131
Line 131: // Load a specific plugin with full path.
On 2009/08/12 03:52:14, abarth wrote:
> static

This can't be static, it depends on ReadPluginInfo and ShouldLoadPlugin which
aren't.

http://codereview.chromium.org/164305/diff/63/74#newcode142
Line 142: // Load all plugins from a specific directory
On 2009/08/12 03:52:14, abarth wrote:
> static

calls LoadPlugin which isn't static.

http://codereview.chromium.org/164305/diff/63/74#newcode149
Line 149: // Load internal plugins.
On 2009/08/12 03:52:14, abarth wrote:
> static

ditto

http://codereview.chromium.org/164305/diff/63/75
File webkit/glue/plugins/plugin_list_win.cc (right):

http://codereview.chromium.org/164305/diff/63/75#newcode304
Line 304: bool IsNewerVersion(const std::wstring& a, const std::wstring& b) {
On 2009/08/12 03:52:14, abarth wrote:
> either put this in an anonymous names space or declare it static.

this isn't part of the change, so I think review comments should just stay to
the modified code..

http://codereview.chromium.org/164305/diff/63/75#newcode322
Line 322: 
On 2009/08/12 03:52:14, abarth wrote:
> DCHECK that we hold the lock

I added a missing autolock.

[abarth-chromium, 2009-08-13 02:27:36]

Ok John.  I've done my best to verify that this code is correct.  I'd prefer
that we used automated tools (like DCHECKs and static) to make it easier to
verify that this code is thread safe, but I'm not going insist.  LGTM.

[M-A Ruel, 2009-08-14 14:33:10]

From:
http://build.chromium.org/buildbot/waterfall/builders/Chromium%20Reliability/...
Will get the partial result of current build r23419.
Results for extended list of web sites:
success: 62356; crashes: 68; crash dumps: 0; timeout: 550

Crash rate increased dramatically at:
http://build.chromium.org/buildbot/waterfall/builders/Chromium%20Reliability/...
Will get the partial result of current build r23421.
Results for extended list of web sites:
success: 57578; crashes: 1374; crash dumps: 0; timeout: 559


Changes:
http://src.chromium.org/viewvc/chrome?view=rev&revision=23420
http://src.chromium.org/viewvc/chrome?view=rev&revision=23421

Feels like 23420 is the culprit. Reverting.