Closed Bug 555086 Opened 11 years ago Closed 9 years ago

NPP_New failure are not handled

Categories

(Core :: IPC, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: BenWa, Assigned: krangam)

Details

(Whiteboard: [inbound])

Attachments

(1 file, 3 obsolete files)

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.
Attached patch fix (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #435020 - Flags: review?
Attachment #435020 - Flags: review? → review?(bent.mozilla)
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.
Attachment #435020 - Flags: review?(bent.mozilla) → review+
Attached patch review comment applied (obsolete) — Splinter Review
Attachment #435020 - Attachment is obsolete: true
Comment on attachment 435050 [details] [diff] [review]
review comment applied

Awesome! Thanks!
Attachment #435050 - Flags: review+
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.
Whiteboard: [good first bug][mentor=benwa]
Assignee: bgirard → nobody
Assignee: nobody → krangam
Patch works for PluginModuleChild.cpp while for PluginModuleParent.cpp someone has already made the fix.
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.
Attachment #547607 - Flags: review?(bent.mozilla)
Sure, I'd do that. But how do I test this? I just applied the patch that was in the attachment.
(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.
(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 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.)
Attachment #547607 - Flags: review?(bent.mozilla)
Updated the patch per your comments.
Attachment #547979 - Flags: review?(jones.chris.g)
Attachment #547607 - Attachment is obsolete: true
Comment on attachment 547979 [details] [diff] [review]
Updated the patch with the comments from Jones

Thanks!
Attachment #547979 - Flags: review?(jones.chris.g) → review+
Keywords: checkin-needed
Attachment #435050 - Attachment is obsolete: true
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
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=benwa] → [inbound]
http://hg.mozilla.org/mozilla-central/rev/f8768a3e3a9f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.