Null check instance_ in PepperWebPluginImpl to prevent re-entry NPE

Null check instance_ in PepperWebPluginImpl to prevent re-entry NPE

This patch fixes many NPEs in PepperWebPluginImpl where Sergeant
Pepper forgot to check the instance before using it. The instance
may be null due to re-entry and a comment has been added about this.

BUG=715747
Review-Url: https://codereview.chromium.org/2835193008
Cr-Commit-Position: refs/heads/master@{#467817}
Committed: https://chromium.googlesource.com/chromium/src/+/c7a827953ea89e6c314956119514fa3acd69eba6

[pdr., 2017-04-26 21:14:16]

pdr@chromium.org changed reviewers:
+ bbudge@chromium.org, raymes@chromium.org

[pdr., 2017-04-26 21:14:25]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 21:15:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 22:35:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 22:35:39]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-04-27 00:37:40]

This looks similar to the case described in
PepperWebPluginImpl::V8ScriptableObject where if you look in the stack trace,
the plugin is being initialized and this call is happening re-entrantly. It's
probably worth adding a comment similar to what is there:

// Re-entrancy may cause JS to try to execute script on the plugin before it
// is fully initialized. See e.g. crbug.com/715747

lgtm

[pdr., 2017-04-27 01:42:40]

On 2017/04/27 at 00:37:40, raymes wrote:
> This looks similar to the case described in
PepperWebPluginImpl::V8ScriptableObject where if you look in the stack trace,
the plugin is being initialized and this call is happening re-entrantly. It's
probably worth adding a comment similar to what is there:
> 
> // Re-entrancy may cause JS to try to execute script on the plugin before it
> // is fully initialized. See e.g. crbug.com/715747
> 
> lgtm

Oh good catch. Digging through the crash logs more, I found more cases of likely
the same bug:
PepperWebPluginImpl::UpdateFocus: go/crash/f6299d4910000000
PepperWebPluginImpl::handleInputEvent: go/crash/2b51ba2e80000000 (many of these)
PepperWebPluginImpl::didFailLoading: go/crash/62ff59d080000000

I think we should go ahead and add the comment + instance_ check to all of the
instance_ uses that might be reachable from re-entry. Does that sound reasonable
to you?

[raymes, 2017-04-27 02:05:33]

On 2017/04/27 01:42:40, pdr. wrote:
> On 2017/04/27 at 00:37:40, raymes wrote:
> > This looks similar to the case described in
> PepperWebPluginImpl::V8ScriptableObject where if you look in the stack trace,
> the plugin is being initialized and this call is happening re-entrantly. It's
> probably worth adding a comment similar to what is there:
> > 
> > // Re-entrancy may cause JS to try to execute script on the plugin before it
> > // is fully initialized. See e.g. crbug.com/715747
> > 
> > lgtm
> 
> Oh good catch. Digging through the crash logs more, I found more cases of
likely
> the same bug:
> PepperWebPluginImpl::UpdateFocus: go/crash/f6299d4910000000
> PepperWebPluginImpl::handleInputEvent: go/crash/2b51ba2e80000000 (many of
these)
> PepperWebPluginImpl::didFailLoading: go/crash/62ff59d080000000
> 
> I think we should go ahead and add the comment + instance_ check to all of the
> instance_ uses that might be reachable from re-entry. Does that sound
reasonable
> to you?

That sounds good, thanks!

[pdr., 2017-04-27 18:12:06]

Description was changed from

==========
Null check instance_ in PepperWebPluginImpl::Paint

This patch fixes a NPE in PepperWebPluginImpl::Paint where Sergeant
Pepper forgot to check the instance; it is checked in most other
callsites in the file.

BUG=715747
==========

to

==========
Null check instance_ in PepperWebPluginImpl to prevent re-entry NPE

This patch fixes many NPEs in PepperWebPluginImpl where Sergeant
Pepper forgot to check the instance before using it. The instance
may be null due to re-entry and a comment has been added about this.

BUG=715747
==========

[pdr., 2017-04-27 19:01:47]

On 2017/04/27 at 02:05:33, raymes wrote:
> On 2017/04/27 01:42:40, pdr. wrote:
> > On 2017/04/27 at 00:37:40, raymes wrote:
> > > This looks similar to the case described in
> > PepperWebPluginImpl::V8ScriptableObject where if you look in the stack
trace,
> > the plugin is being initialized and this call is happening re-entrantly.
It's
> > probably worth adding a comment similar to what is there:
> > > 
> > > // Re-entrancy may cause JS to try to execute script on the plugin before
it
> > > // is fully initialized. See e.g. crbug.com/715747
> > > 
> > > lgtm
> > 
> > Oh good catch. Digging through the crash logs more, I found more cases of
likely
> > the same bug:
> > PepperWebPluginImpl::UpdateFocus: go/crash/f6299d4910000000
> > PepperWebPluginImpl::handleInputEvent: go/crash/2b51ba2e80000000 (many of
these)
> > PepperWebPluginImpl::didFailLoading: go/crash/62ff59d080000000
> > 
> > I think we should go ahead and add the comment + instance_ check to all of
the
> > instance_ uses that might be reachable from re-entry. Does that sound
reasonable
> > to you?
> 
> That sounds good, thanks!

Done! PTAL

[pdr., 2017-04-27 19:58:40]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 19:59:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 20:20:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 20:20:34]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2017-04-27 22:00:52]

(compile failures are an ongoing infrastructure failure)

[raymes, 2017-04-27 23:36:19]

lgtm

[pdr., 2017-04-28 01:42:21]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-04-28 01:43:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 01:51:46]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493343741074050,
"parent_rev": "ea1226095c8fe84ea661a5fed649004686284f32", "commit_rev":
"c7a827953ea89e6c314956119514fa3acd69eba6"}

[commit-bot: I haz the power, 2017-04-28 01:51:54]

Description was changed from

==========
Null check instance_ in PepperWebPluginImpl to prevent re-entry NPE

This patch fixes many NPEs in PepperWebPluginImpl where Sergeant
Pepper forgot to check the instance before using it. The instance
may be null due to re-entry and a comment has been added about this.

BUG=715747
==========

to

==========
Null check instance_ in PepperWebPluginImpl to prevent re-entry NPE

This patch fixes many NPEs in PepperWebPluginImpl where Sergeant
Pepper forgot to check the instance before using it. The instance
may be null due to re-entry and a comment has been added about this.

BUG=715747

Review-Url: https://codereview.chromium.org/2835193008
Cr-Commit-Position: refs/heads/master@{#467817}
Committed:
https://chromium.googlesource.com/chromium/src/+/c7a827953ea89e6c314956119514...
==========

[commit-bot: I haz the power, 2017-04-28 01:51:55]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c7a827953ea89e6c314956119514...