333387 - [@ ns4xPluginInstance::Print]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[timeless]

found by coverity

i wonder if we were really supposed to be checking something other than fCallbacks

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 242421 [details] [diff] [review]
Patch v1

>-  if(fCallbacks) {
>+  if(fCallbacks->print) {

I don't think we want to change this. Not that I can imagine how this could break any existing plugins, but you never really know, and there's really not much benefit in changing this here...

[...]
+    NS_TRY_SAFE_CALL_VOID(CallNPP_PrintProc(fCallbacks->print,
+                                            &fNPP,
+                                            thePrint), fLibrary, this);
   }
 
-  NS_TRY_SAFE_CALL_VOID(CallNPP_PrintProc(fCallbacks->print,
-                                          &fNPP,
-                                          thePrint), fLibrary, this);
-

But this should be inside a if (fCallbacks->print) check. r+sr=jst with that changed.

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Patch v2 too Johnny Stenback's comments. Do not alter the if check.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 243532 [details] [diff] [review]
Patch v2

+    NS_TRY_SAFE_CALL_VOID(CallNPP_PrintProc(fCallbacks->print,
+                                            &fNPP,
+                                            thePrint), fLibrary, this);
   }
 
-  NS_TRY_SAFE_CALL_VOID(CallNPP_PrintProc(fCallbacks->print,
-                                          &fNPP,
-                                          thePrint), fLibrary, this);

Unless I missed something the problem coverity was complaining about was that we were accessing fCallbacks->print w/o null checking it first. If so, then this change won't fix that problem.

As I see this we want the above NS_TRY_SAFE_CALL_VOID() to remain outside the above if but we want it inside a if (fCallbacks->print) check. And if that's what we want we really could remove the above check that checks fCallbacks since it's a pointless check if we access fCallbacks here w/o the check. No other users of fCallbacks members check if fCallbacks is null or not before use, so loosing that check here too should be safe.

>   NPP_PLUGIN_LOG(PLUGIN_LOG_NORMAL,
>   ("NPP PrintProc called: this=%p, pDC=%p, [x=%d,y=%d,w=%d,h=%d], clip[t=%d,b=%d,l=%d,r=%d]\n",
>   this,
>   platformPrint->print.embedPrint.platformPrint,
>   platformPrint->print.embedPrint.window.x,
>   platformPrint->print.embedPrint.window.y,
>   platformPrint->print.embedPrint.window.width,
>   platformPrint->print.embedPrint.window.height,

Comment 5

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3

Patch v3 (corrected of #2).

Sorry I misunderstood what you said, corrected too comments and moved |NS_TRY_SAFE_CALL_VOID| call inside a |fCallbacks->print| if check.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 243539 [details] [diff] [review]
Patch v3

+  if(fCallbacks->print)
+      NS_TRY_SAFE_CALL_VOID(CallNPP_PrintProc(fCallbacks->print,
+                                              &fNPP,
+                                              thePrint), fLibrary, this);

r+sr=jst, though I'd put braces around that even if it's a one-liner.

Thanks for fixing!

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

Attachment: For checkin

Final patch, added braces as suggested in comment #6.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp 	1.134