555086 - NPP_New failure are not handled

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Benoit Girard (:BenWa)]

See the patch for details:
Diff 1: The error code is not reported when your return false; you get a ipc message error.
Diff 2: NPP_Destroy is called when NPP_New returns an error.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: fix

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 435020 [details] [diff] [review]
fix

>diff --git a/dom/plugins/PluginModuleChild.cpp b/dom/plugins
> ...
>     if (*error != NPERR_NO_ERROR) {
>-        NPP_Destroy(instance, 0);
>         return *error;
>     }

Not your fault, but returning *error is incorrect here. Please make that return NS_ERROR_FAILURE instead.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: review comment applied

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 435050 [details] [diff] [review]
review comment applied

Awesome! Thanks!

Comment 5

[Benoit Girard (:BenWa)]

There's already a patch for this but we need to fix bit rot and make sure the change is still needed. Good opportunity for some one to learn compiling, debugging, submitting a patch.

Willing to mentor if someone is interested in taking this.

Comment 6

[krangam]

Patch works for PluginModuleChild.cpp while for PluginModuleParent.cpp someone has already made the fix.

Comment 7

[Benoit Girard (:BenWa)]

Cool, just post an updated patch and attach it here:
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f

Comment 8

[krangam]

Attachment: Updated the directories and file names in the attached patch. This works with the current version

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 547607 [details] [diff] [review]
Updated the directories and file names in the attached patch. This works with the current version

Same change as before, can we carry foward the r+?

krangam, if you include the [diff] section from that link in your hgrc you'll get a more descriptive patch that makes the process easier for large changes. No need for this patch here since it's simple.

Once it's reviewed someone with commit access will check it in.

Comment 10

[krangam]

Sure, I'd do that. But how do I test this? I just applied the patch that was in the attachment.

Comment 11

[Benoit Girard (:BenWa)]

(In reply to comment #10)
> Sure, I'd do that. But how do I test this? I just applied the patch that was
> in the attachment.

You'll want to make sure it builds at the very least on the most relevant platform, in this case any platform. 

For this particular change you'd have to recompile the test plugin to return an error code in its constructor and verify that the test plugin is unloaded. I had already done that previously so I think we'd be safe without retesting that.

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to comment #9)
> Same change as before, can we carry foward the r+?

It's a good idea to re-review this. The original patch only had 3 lines of context and no -p info, so it's hard to know if patch re-applied it in the right place.

This is a super-trivial review for anyone familiar with the code. Let's try and get this reviewed today. Bent is on vacation, so I'll have to look and see who recently touched this file...

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 547607 [details] [diff] [review]
Updated the directories and file names in the attached patch. This works with the current version

>diff --git a/dom/plugins/ipc/PluginModuleChild.cpp b/dom/plugins/ipc/PluginModuleChild.cpp
>--- a/dom/plugins/ipc/PluginModuleChild.cpp
>+++ b/dom/plugins/ipc/PluginModuleChild.cpp
>@@ -1578,9 +1578,6 @@
>                           argn,
>                           argv,
>                           0);
>-    if (NPERR_NO_ERROR != *rv) {
>-        return false;
>-    }

This code has now changed to the point where this patch isn't /quite/ right, but almost there.  What's happening now is,

  *rv = mFunctions.newp(...);
   if (NPERR_NO_ERROR != *rv) {
       return false;
   }
#if MAC
    [Mac-specific setup code]
#endif

If we remove the early-return, we'll end up doing the Mac-specific setup on an invalid instance, which isn't what we want.  Instead of removing the error-check and early return, what I think we should do instead is just have a |return true;| early-return, that is change the |false| to |true| in the existing error check.

Thanks!  Let me know if I can help more.

(I cleared the review request to indicate that a new patch is needed.)

Comment 14

[krangam]

Attachment: Updated the patch with the comments from Jones

Updated the patch per your comments.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 547979 [details] [diff] [review]
Updated the patch with the comments from Jones

Thanks!

Comment 16

[Jason Orendorff [:jorendorff]]

Karthik, I checked pushed this to mozilla-inbound. This bug should be marked "fixed" later today.

http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f8768a3e3a9f

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/f8768a3e3a9f