Fix check for removed plug-in while loading a blocked plug-in.

Fix check for removed plug-in while loading a blocked plug-in.

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

[Bernhard Bauer, 2013-08-29 17:50:55]

Please review.

This seems to fix it, but I'm not sure whether I can add a regression test.
ClickToPlayPluginTest.DeleteSelfAtLoad seems to test the same thing, but I can't
get it to crash, and I don't know how to test for success with the repro case
where it does crash (we don't seem to have browser tests that test for the
absence of a crash, and in any case that seems unreliable to me :-/ ).

[jln (very slow on Chromium), 2013-08-29 18:54:30]

On 2013/08/29 17:50:55, Bernhard Bauer wrote:
> Please review.
> 
> This seems to fix it, but I'm not sure whether I can add a regression test.
> ClickToPlayPluginTest.DeleteSelfAtLoad seems to test the same thing, but I
can't
> get it to crash, and I don't know how to test for success with the repro case
> where it does crash (we don't seem to have browser tests that test for the
> absence of a crash, and in any case that seems unreliable to me :-/ ).

This lgtm, but I'm not a good reviewer for this. Please add someone more
familiar wit this area if you want a second opinion.

In a way, any browser_tests tests for the absence of a crash. It would be nice
if you could land a unittest that reproduces this particular case (it may only
crash reliably on ASAN builds).

[jln (very slow on Chromium), 2013-08-29 19:12:53]

On 2013/08/29 18:54:30, Julien Tinnes wrote:
> On 2013/08/29 17:50:55, Bernhard Bauer wrote:
> > Please review.
> > 
> > This seems to fix it, but I'm not sure whether I can add a regression test.
> > ClickToPlayPluginTest.DeleteSelfAtLoad seems to test the same thing, but I
> can't
> > get it to crash, and I don't know how to test for success with the repro
case
> > where it does crash (we don't seem to have browser tests that test for the
> > absence of a crash, and in any case that seems unreliable to me :-/ ).
> 
> This lgtm, but I'm not a good reviewer for this. Please add someone more
> familiar wit this area if you want a second opinion.
> 
> In a way, any browser_tests tests for the absence of a crash. It would be nice
> if you could land a unittest that reproduces this particular case (it may only
> crash reliably on ASAN builds).

Note: I don't know how this area is tested, so if it looks like it'd require a
large effort to add testing for arbitrary html loading plugins, I think it'd be
ok to punt and rely on ClusterFuzz instead.

Thanks for fixing this!

[Bernhard Bauer, 2013-08-29 19:20:29]

+John

[jam, 2013-08-29 20:36:18]

lgtm

[commit-bot: I haz the power, 2013-08-29 22:06:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/23771002/1

[commit-bot: I haz the power, 2013-08-29 23:56:25]

Change committed as 220441