90389 - ns4xPluginInstance should not check for pdata != NULL.

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Brian Nesse (gone)]

There are currently a number of places in ns4xPluginInstance where the plug-in 
private data member (pdata) of the plugin instance handle (NPP structure) is 
tested against NULL and NS_ERROR_FAILURE is returned if it is NULL.

As this member is never accessed by us, other than when we initialize it to NULL, 
(rightfully so as it is plugin private information) we should not be throwing 
errors if it is NULL. If a vendor writes a plugin that never uses this field, 
their plugin will fail to work in Mozilla/Netscape 6 because we will throw an 
error on the NULL check.

Comment 1

[Brian Nesse (gone)]

Attachment: Patch to remove null checks on pdata.

Comment 2

[av (gone)]

There was a reason to add this check as it helped to resolve a lot of crashes. I 
beleive we should still check for it but may be you are right -- not return 
error.

Comment 3

[Peter Lubczynski]

After looking at this with Brian, I tend to agree with him. According to the
spec, Navigator shouldn't ever care what's in pdata as this is private to the
plugin. I think this is a side-effect that when pdata is null the plugin isn't
running. I think we were using this to prevent random crashes. But, this isn't a
very reliable way of checking if the plugin is running and should be removed. If
a plugin crashes, we should try to find the root cause rather than trying to
rely on this hack.

Comment 4

[Brian Nesse (gone)]

I've been running with this patch in my tree for a week and have tested it 
against every plugin I have. I have seen no related crashes.

I can't really see how this would protect us against crashes because, as I noted 
above, we never access pdata. If the plugin is crashing because they are 
accessing their own private data without validating it, they need to revisit 
their own code.

Besides which if we do check it, we have 2 options, 1) call the plugin with it 
being null, or not calling the plugin... which I have already suggested is 
incorrect.

Comment 5

[av (gone)]

I agree with this, but there is also a matter of taking responsibility for 
introducing a possible flood of crashers which will look like regressions. May 
it is right time to do it for the trunk only, so we can see how it is cooking 
over time.

Comment 6

[Peter Lubczynski]

I think the same. This is totaly great for the trunk, r=peterl, but not for the
branch. Although I'm pretty sure this is the right thing to do, I think the risk
of regressions, especially with Shrirang on vacation, is too high to put this in
the branch.

Comment 7

[Brian Nesse (gone)]

Reassign to myself and add keywords.

Comment 8

[Marc Attinasi]

sr=attinasi for the patch 41994

Comment 9

[Brian Nesse (gone)]

I am in total agreement that this shouldn't go on the branch. I had no intention 
of trying to land it there. For that matter I'll hold off on checking it in to 
the trunk for a while if you guys want to run with the patch installed in your 
trees for a bit first.

Comment 10

[av (gone)]

I don't know, I would land it on the trunk -- the earlier we see the problems 
the better.

Comment 11

[Peter Lubczynski]

I'd wait for Shrirang to get back next week before landing it but I don't think
it'll hurt anything. Brian, have you run with this on Win32 too?

Comment 12

[Brian Nesse (gone)]

No, I just finished reformatting my HD and reinstalling Win2k, and all the build 
tools. Of course this also means I don't have any plugins installed either...

Comment 13

[Brian Nesse (gone)]

Fix checked in.

Comment 14

[shrirang khanzode]

v(stamped)