NPP_New failure are not handled

RESOLVED FIXED in mozilla8

Status

()

Core
IPC
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: BenWa, Assigned: krangam)

Tracking

Trunk
mozilla8
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 435020 [details] [diff] [review]
fix
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #435020 - Flags: review?
(Reporter)

Updated

8 years ago
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+
(Reporter)

Comment 3

8 years ago
Created attachment 435050 [details] [diff] [review]
review comment applied
Attachment #435020 - Attachment is obsolete: true
Comment on attachment 435050 [details] [diff] [review]
review comment applied

Awesome! Thanks!
Attachment #435050 - Flags: review+
(Reporter)

Comment 5

6 years ago
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]
(Reporter)

Updated

6 years ago
Assignee: bgirard → nobody
Assignee: nobody → krangam
(Assignee)

Comment 6

6 years ago
Patch works for PluginModuleChild.cpp while for PluginModuleParent.cpp someone has already made the fix.
(Reporter)

Comment 7

6 years ago
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
(Assignee)

Comment 8

6 years ago
Created attachment 547607 [details] [diff] [review]
Updated the directories and file names in the attached patch. This works with the current version
(Reporter)

Comment 9

6 years ago
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)
(Assignee)

Comment 10

6 years ago
Sure, I'd do that. But how do I test this? I just applied the patch that was in the attachment.
(Reporter)

Comment 11

6 years ago
(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)
(Assignee)

Comment 14

6 years ago
Created attachment 547979 [details] [diff] [review]
Updated the patch with the comments from Jones

Updated the patch per your comments.
Attachment #547979 - Flags: review?(jones.chris.g)
(Reporter)

Updated

6 years ago
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+
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
(Reporter)

Updated

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.