Last Comment Bug 555086 - NPP_New failure are not handled
: NPP_New failure are not handled
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla8
Assigned To: krangam
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-25 16:00 PDT by Benoit Girard (:BenWa)
Modified: 2011-07-26 03:58 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (670 bytes, patch)
2010-03-25 16:01 PDT, Benoit Girard (:BenWa)
bent.mozilla: review+
Details | Diff | Splinter Review
review comment applied (723 bytes, patch)
2010-03-25 16:56 PDT, Benoit Girard (:BenWa)
bent.mozilla: review+
Details | Diff | Splinter Review
Updated the directories and file names in the attached patch. This works with the current version (385 bytes, patch)
2011-07-21 22:05 PDT, krangam
no flags Details | Diff | Splinter Review
Updated the patch with the comments from Jones (1.06 KB, patch)
2011-07-23 19:38 PDT, krangam
cjones.bugs: review+
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2010-03-25 16:00:08 PDT
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) 2010-03-25 16:01:10 PDT
Created attachment 435020 [details] [diff] [review]
fix
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-03-25 16:48:33 PDT
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) 2010-03-25 16:56:55 PDT
Created attachment 435050 [details] [diff] [review]
review comment applied
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-03-25 17:40:40 PDT
Comment on attachment 435050 [details] [diff] [review]
review comment applied

Awesome! Thanks!
Comment 5 Benoit Girard (:BenWa) 2011-05-25 07:29:22 PDT
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 2011-07-21 21:45:22 PDT
Patch works for PluginModuleChild.cpp while for PluginModuleParent.cpp someone has already made the fix.
Comment 7 Benoit Girard (:BenWa) 2011-07-21 21:50:04 PDT
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 2011-07-21 22:05:17 PDT
Created attachment 547607 [details] [diff] [review]
Updated the directories and file names in the attached patch. This works with the current version
Comment 9 Benoit Girard (:BenWa) 2011-07-21 22:10:55 PDT
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 2011-07-21 22:34:01 PDT
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) 2011-07-21 23:04:21 PDT
(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] 2011-07-22 07:01:01 PDT
(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 2011-07-22 10:38:08 PDT
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 2011-07-23 19:38:32 PDT
Created attachment 547979 [details] [diff] [review]
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 2011-07-25 07:14:46 PDT
Comment on attachment 547979 [details] [diff] [review]
Updated the patch with the comments from Jones

Thanks!
Comment 16 Jason Orendorff [:jorendorff] 2011-07-25 12:57:07 PDT
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] 2011-07-26 03:58:26 PDT
http://hg.mozilla.org/mozilla-central/rev/f8768a3e3a9f

Note You need to log in before you can comment on or make changes to this bug.