Closed Bug 333387 Opened 14 years ago Closed 14 years ago

[@ ns4xPluginInstance::Print]

Categories

(Core :: Plug-ins, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: timeless, Assigned: sciguyryan)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, crash)

Crash Data

Attachments

(1 file, 3 obsolete files)

found by coverity

i wonder if we were really supposed to be checking something other than fCallbacks
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → sciguyryan+bugzilla
Status: NEW → ASSIGNED
Attachment #242421 - Flags: superreview?(jst)
Attachment #242421 - Flags: review?(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.
Attachment #242421 - Flags: superreview?(jst)
Attachment #242421 - Flags: superreview-
Attachment #242421 - Flags: review?(jst)
Attachment #242421 - Flags: review-
Attached patch Patch v2 (obsolete) — Splinter Review
Patch v2 too Johnny Stenback's comments. Do not alter the if check.
Attachment #242421 - Attachment is obsolete: true
Attachment #243532 - Flags: superreview?(jst)
Attachment #243532 - Flags: review?(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,
Attachment #243532 - Flags: superreview?(jst)
Attachment #243532 - Flags: superreview-
Attachment #243532 - Flags: review?(jst)
Attachment #243532 - Flags: review-
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #243532 - Attachment is obsolete: true
Attachment #243539 - Flags: superreview?(jst)
Attachment #243539 - Flags: review?(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!
Attachment #243539 - Flags: superreview?(jst)
Attachment #243539 - Flags: superreview+
Attachment #243539 - Flags: review?(jst)
Attachment #243539 - Flags: review+
Attached patch For checkinSplinter Review
Final patch, added braces as suggested in comment #6.
Attachment #243539 - Attachment is obsolete: true
Whiteboard: [checkin needed]
mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp 	1.134
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Crash Signature: [@ ns4xPluginInstance::Print]
You need to log in before you can comment on or make changes to this bug.