Be more thorough checking for NULL NPP values

Be more thorough checking for NULL NPP values

Unlike the other entry points, NPN_GetValue and NPN_SetValue don't check
for NULL npp values. nspluginwrapper will happily call functions with
NULL npp if the instances have been destroyed in the meantime. (And the
patch for #53940 will make this happen more often to avoid a plugin-side
crash.)

NOTE: This does not fix #53940, but the fix for it in nspluginwrapper reveals
some missing checks on our end.

BUG=53940
TEST=none

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

[davidben, 2011-03-23 18:24:15]

A little patch to be slightly more robust against silly plugins. It doesn't
actually fix the linked bug; the nspluginwrapper patch is still needed for it.
But I noticed a few sites were still crashing. Apparently the problem was that
nspluginwrapper never does a NULL check for the npp pointer itself; it just
happily passes a NULL plugin instance to NPN_* and hopes the browser will check.
We did not in NPN_GetValue/NPN_SetValue.

I can adjust the nspluginwrapper patch to also do the checks if they're better
done there, but it seems some variables do allow NULL npp and some don't, and
I'd rather not maintain a list in nspluginwrapper too. We seem to be careful
elsewhere, and a brief look at Mozilla code suggests they are too, so the
Chromium-side checks are probably worthwhile.

[ananta, 2011-03-23 18:39:26]

LGTM

http://codereview.chromium.org/6722021/diff/2001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_host.cc (right):

http://codereview.chromium.org/6722021/diff/2001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_host.cc:696: if (!plugin.get()) {
Should we move this code outside the switch block to reduce the clutter?

[davidben, 2011-03-23 18:51:08]

http://codereview.chromium.org/6722021/diff/2001/webkit/plugins/npapi/plugin_...
File webkit/plugins/npapi/plugin_host.cc (right):

http://codereview.chromium.org/6722021/diff/2001/webkit/plugins/npapi/plugin_...
webkit/plugins/npapi/plugin_host.cc:696: if (!plugin.get()) {
On 2011/03/23 18:39:26, ananta wrote:
> Should we move this code outside the switch block to reduce the clutter?

Some variables don't seem to require a plugin instance. For instance,
NPNVjavascriptEnabledBool and NPNVSupportsWindowless. I don't know if
"officially" you're allowed to query them with a null instance, or if plugins
actually do, but we and Mozilla both accept it, so it's conceivable that someone
does.

(Hence why I'm not very thrilled about doing the check on nspluginwrapper's end,
if it does in fact depend on the particular variable.)

[ananta, 2011-03-23 23:39:18]

On 2011/03/23 18:51:08, David Benjamin wrote:
>
http://codereview.chromium.org/6722021/diff/2001/webkit/plugins/npapi/plugin_...
> File webkit/plugins/npapi/plugin_host.cc (right):
> 
>
http://codereview.chromium.org/6722021/diff/2001/webkit/plugins/npapi/plugin_...
> webkit/plugins/npapi/plugin_host.cc:696: if (!plugin.get()) {
> On 2011/03/23 18:39:26, ananta wrote:
> > Should we move this code outside the switch block to reduce the clutter?
> 
> Some variables don't seem to require a plugin instance. For instance,
> NPNVjavascriptEnabledBool and NPNVSupportsWindowless. I don't know if
> "officially" you're allowed to query them with a null instance, or if plugins
> actually do, but we and Mozilla both accept it, so it's conceivable that
someone
> does.
> 
> (Hence why I'm not very thrilled about doing the check on nspluginwrapper's
end,
> if it does in fact depend on the particular variable.)

ok. LGTM