PluginLoaderPosix: Fix an off by one error.

PluginLoaderPosix: Fix the case where the utility process crashes after all plugins have been loaded.

This was causing a crash on load on my system.

BUG=111935
TEST=none
R=rsesek

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120098

[James Hawkins, 2012-01-29 00:57:41]

The following tests are failing with this change:
PluginLoaderPosixTest.CrashedProcess, PluginLoaderPosixTest.AllCrashed.

It's not clear from either the code or the test what the right behavior is.

Here's the problem this CL fixes: once all plugins are loaded, next_load_index_
== canonical_list_.size().  OnProcessCrashed() erases n items from the list,
where n == |next_load_index_ + 1| == |canonical_list_.size() + 1|.  Boom, we're
erasing past the end of the list.

It seems AllCrashed is not actually testing this code path since the first two
plugin loads are mocked, thus |next_load_index_| is not correct.

[Robert Sesek, 2012-01-30 16:45:41]

I'm fairly certain that this code is correct. If anything, OnProcessedCrashed()
may need a range check, but the +1 is designed to skip over the plugin that was
just attempted to load if it crashed while doing so.

This also needs a bug filed for it with a stack trace. Is this happening in the
field? Are there other reports of it? Why did this just start happening now?

https://chromiumcodereview.appspot.com/9297049/diff/1/content/browser/plugin_...
File content/browser/plugin_loader_posix_unittest.cc (right):

https://chromiumcodereview.appspot.com/9297049/diff/1/content/browser/plugin_...
content/browser/plugin_loader_posix_unittest.cc:213:
plugin_loader()->OnProcessCrashed(42);
Why would this be happening after the callbacks are run?

[James Hawkins, 2012-01-30 17:56:58]

On 2012/01/30 16:45:41, rsesek wrote:
> I'm fairly certain that this code is correct. If anything,
OnProcessedCrashed()
> may need a range check, but the +1 is designed to skip over the plugin that
was
> just attempted to load if it crashed while doing so.
> 

OK that makes sense up to the point when all the plugins are loaded. The
UtilityProcessHost can crash after that point, so perhaps a size check is the
best solution.

> This also needs a bug filed for it with a stack trace. Is this happening in
the
> field? Are there other reports of it? Why did this just start happening now?
> 

Filed crbug.com/111935 with stack traces.  I'm not good with crash/ but I think
my query returned ~75 crashed on Linux.  I'm not particularly sure why this
started happening for me, but my hypothesis is that it is caused by some 'bad'
media plugins I installed recently for random media types.

>
https://chromiumcodereview.appspot.com/9297049/diff/1/content/browser/plugin_...
> File content/browser/plugin_loader_posix_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9297049/diff/1/content/browser/plugin_...
> content/browser/plugin_loader_posix_unittest.cc:213:
> plugin_loader()->OnProcessCrashed(42);
> Why would this be happening after the callbacks are run?

[James Hawkins, 2012-01-30 17:58:21]

http://codereview.chromium.org/9297049/diff/1/content/browser/plugin_loader_p...
File content/browser/plugin_loader_posix_unittest.cc (right):

http://codereview.chromium.org/9297049/diff/1/content/browser/plugin_loader_p...
content/browser/plugin_loader_posix_unittest.cc:213:
plugin_loader()->OnProcessCrashed(42);
On 2012/01/30 16:45:41, rsesek wrote:
> Why would this be happening after the callbacks are run?

I don't know; I wrote the test in an attempt to reproduce the crash (which it
does).

[James Hawkins, 2012-01-31 22:55:59]

Updated the fix to special case when all plugins have loaded.

[Robert Sesek, 2012-01-31 23:16:51]

Please also update the CL description to reflect the bug that this is actually
fixing.

It'd be nice to know the actual cause of this issue, but this fix is
appropriate.

http://codereview.chromium.org/9297049/diff/9001/content/browser/plugin_loade...
File content/browser/plugin_loader_posix.cc (right):

http://codereview.chromium.org/9297049/diff/9001/content/browser/plugin_loade...
content/browser/plugin_loader_posix.cc:47: if (next_load_index_ ==
canonical_list_.size()) {
nit: Add a comment referencing this bug.

[Robert Sesek, 2012-01-31 23:17:00]

And by that I mean LGTM.

[James Hawkins, 2012-01-31 23:28:22]

http://codereview.chromium.org/9297049/diff/9001/content/browser/plugin_loade...
File content/browser/plugin_loader_posix.cc (right):

http://codereview.chromium.org/9297049/diff/9001/content/browser/plugin_loade...
content/browser/plugin_loader_posix.cc:47: if (next_load_index_ ==
canonical_list_.size()) {
On 2012/01/31 23:16:51, rsesek wrote:
> nit: Add a comment referencing this bug.

Done.

[Robert Sesek, 2012-02-01 00:19:10]

http://codereview.chromium.org/9297049/diff/9001/content/browser/plugin_loade...
File content/browser/plugin_loader_posix.cc (right):

http://codereview.chromium.org/9297049/diff/9001/content/browser/plugin_loade...
content/browser/plugin_loader_posix.cc:47: if (next_load_index_ ==
canonical_list_.size()) {
On 2012/01/31 23:28:22, James Hawkins wrote:
> On 2012/01/31 23:16:51, rsesek wrote:
> > nit: Add a comment referencing this bug.
> 
> Done.

FYI this isn't on the latest patch set if you intend to CQ.