PRBool misuse bugs in content/

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
DOM
P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: (dormant account), Assigned: (dormant account))

Tracking

Trunk
mozilla1.9beta4
x86
Linux
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 283393 [details] [diff] [review]
Straightforward fixes

This is a manually corrected version of the prcheck-produced patch.
Attachment #283393 - Flags: review?(jonas)
I prefer !!(expr) syntax, but Jonas may have different opinion.
(Assignee)

Comment 2

10 years ago
I think !! is a more compact, but so far the general consensus has been that 0 != is more obvious. I don't have any attachment towards either. I'd be happy change the patch to whatever you guys agree on.
Status: NEW → ASSIGNED
Assignee: nobody → tglek
Status: ASSIGNED → NEW
So I prefer !! and so does jst, so I say lets do that. If that's hard, then please do |expr != 0| rather than |0 != expr|.

Also, the first change is technically not needed as PARENT_BIT_INDOCUMENT is 1, so the result of the boolean & can only be 0 or 1.
(Assignee)

Comment 4

10 years ago
Created attachment 283641 [details] [diff] [review]
Straightforward fixes. take 2

Jonas,
Thanks for the review. 
The first change was a bug in prcheck. 
As for !!, I updated the patch and prcheck to use that instead of 0 !=.
How do you like the updated patch?
Attachment #283393 - Attachment is obsolete: true
Attachment #283393 - Flags: review?(jonas)
Comment on attachment 283641 [details] [diff] [review]
Straightforward fixes. take 2

>Index: content/events/src/nsIMEStateManager.cpp
>@@ -261,17 +261,17 @@ nsIMEStateManager::SetIMEState(nsPresCon
>                                nsIKBStateControl* aKB)
> {
>   if (aState & nsIContent::IME_STATUS_MASK_ENABLED) {
>     PRUint32 state =
>       nsContentUtils::GetKBStateControlStatusFromIMEStatus(aState);
>     aKB->SetIMEEnabled(state);
>   }
>   if (aState & nsIContent::IME_STATUS_MASK_OPENED) {
>-    PRBool open = (aState & nsIContent::IME_STATUS_OPEN);
>+    PRBool open = (!!(aState & nsIContent::IME_STATUS_OPEN));

Feel free to remove the extra parenthesis here.

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>@@ -1122,17 +1122,17 @@ nsGenericHTMLElement::InNavQuirksMode(ns
> }
> 
> void
> nsGenericHTMLElement::UpdateEditableState()
> {
>   // XXX Should we do this only when in a document?
>   ContentEditableTristate value = GetContentEditableValue();
>   if (value != eInherit) {
>-    SetEditableFlag(value);
>+    SetEditableFlag(!!value);

Hmm.. tricky, the tristate can only be 0 or 1 here since the only other valid value (eInherit) has been tested for. I understand that that is hard for prcheck to detect though. I'm fine with leaving this as is, or checking in the fix in order for it not to show up as a false-positive when running prcheck.

Same thing in UpdateEditableFormControlState.


>Index: content/xbl/src/nsXBLPrototypeHandler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp,v
>retrieving revision 1.126
>diff -u -p -8 -r1.126 nsXBLPrototypeHandler.cpp
>--- content/xbl/src/nsXBLPrototypeHandler.cpp	5 Sep 2007 01:55:55 -0000	1.126
>+++ content/xbl/src/nsXBLPrototypeHandler.cpp	3 Oct 2007 17:39:57 -0000
>@@ -224,18 +224,18 @@ nsXBLPrototypeHandler::ExecuteHandler(ns
>     // mHandlerElement and mHandlerText to be null
>     rv = NS_OK;
>   }
> 
>   if (!mHandlerElement) // This works for both types of handlers.  In both cases, the union's var should be defined.
>     return rv;
> 
>   // See if our event receiver is a content node (and not us).
>-  PRBool isXULKey = (mType & NS_HANDLER_TYPE_XUL);
>-  PRBool isXBLCommand = (mType & NS_HANDLER_TYPE_XBL_COMMAND);
>+  PRBool isXULKey = (!!(mType & NS_HANDLER_TYPE_XUL));
>+  PRBool isXBLCommand = (!!(mType & NS_HANDLER_TYPE_XBL_COMMAND));

No need for the outer parans here either.

>Index: content/xslt/src/xslt/txCurrentFunctionCall.cpp
> CurrentFunctionCall::isSensitiveTo(ContextSensitivity aContext)
> {
>-    return (aContext & PRIVATE_CONTEXT);
>+    return (!!(aContext & PRIVATE_CONTEXT));
> }

And here.

>Index: content/xslt/src/xslt/txExecutionState.cpp
>@@ -369,17 +369,17 @@ txExecutionState::pushBool(PRBool aBool)
> 
> PRBool
> txExecutionState::popBool()
> {
>     NS_ASSERTION(mBoolStack.Length(), "popping from empty stack");
>     PRUint32 last = mBoolStack.Length() - 1;
>     NS_ENSURE_TRUE(last != (PRUint32)-1, PR_FALSE);
> 
>-    PRBool res = mBoolStack.ElementAt(last);
>+    PRBool res = !!mBoolStack.ElementAt(last);

Hmm.. this is an array of PRPackedBool which ideally should never be anything other than 0 or 1 either. Is that something that should be fixed in prcheck? I guess you should then enforce that too?

r/sr=me

We should really land this for 1.9 since there are some important fixes in here.
(Assignee)

Comment 6

10 years ago
Created attachment 283746 [details] [diff] [review]
Straightforward fixes. take 3

Jonas,
Sorry about redundant ()s. I realize that the *Tristate vars are guarded and therefore safe. However this analysis isn't flow-sensitive, so that information is lost to prcheck. So by "leaving as is" do you mean you are ok with rewriting it to !!value? Leaving a few false-positives is ok by me. 
The mBoolStack.ElementAt(last) issue is a more serious. I spend a great deal of effort to track typedef information in prcheck because elsa's type system does not preserve typedefs. In this case the type is a result of a template instantiation so there is little I can do to hack around the elsa limitation. I could special-case the Mozilla arrays, but I don't believe that's worth the hassle. Anyways, sorry for not removing this one during my manual inspection and leaving it out of the patch.
Also, I looked through the code that wasn't automatically modified and added 3 more hunks. Affected are: nsIMEStateManager::IsActive(nsPresContext* aPresContext), TX_XSLTFunctionAvailable and nsHTMLDocument::GetBodyContent()
Attachment #283641 - Attachment is obsolete: true
Attachment #283746 - Flags: review?(jonas)
(Assignee)

Comment 7

10 years ago
Jonas: ping.
Could you please review the patch.
Comment on attachment 283746 [details] [diff] [review]
Straightforward fixes. take 3

>Index: content/events/src/nsEventStateManager.h
>   nsresult DoScrollText(nsPresContext* aPresContext,
>                         nsIFrame* aTargetFrame,
>                         nsInputEvent* aEvent,
>                         PRInt32 aNumLines,
>-                        PRBool aScrollHorizontal,
>+                        PRUint8 aScrollHorizontal,

This doesn't seem right. Why don't we want a PRBool here? Same in the .cpp file.

>Index: content/html/content/src/nsGenericHTMLElement.cpp

> nsGenericHTMLElement::UpdateEditableState()
> {
>   // XXX Should we do this only when in a document?
>   ContentEditableTristate value = GetContentEditableValue();
>   if (value != eInherit) {
>-    SetEditableFlag(value);
>+    SetEditableFlag(!!value);

IMHO the old code was fine too. But it's no biggie so I'm ok with making this change too to avoid having it show up as a false positive.

Same in UpdateEditableFormControlState


>Index: content/xbl/src/nsXBLPrototypeHandler.cpp
>-  PRBool isXULKey = (mType & NS_HANDLER_TYPE_XUL);
>-  PRBool isXBLCommand = (mType & NS_HANDLER_TYPE_XBL_COMMAND);
>+  PRBool isXULKey = !!(mType & NS_HANDLER_TYPE_XUL);
>+  PRBool isXBLCommand = !!(mType & NS_HANDLER_TYPE_XBL_COMMAND);

I'm sort of on the fence here what the right thing to do for these are. We could do what the above patch does, or we could turn them into PRUint32s, or we could detect that these are only used in contexts where we check them for 0 (|if (isXULKey) {... |) and then ignore them.

Neither is very appealing.

I guess since this happen rarely enough we might as well do what the patch does. Seems like the simplest.

Sorry for taking so long with the review :(


I definitely think we should take this for the next beta. It fixes some odd bugs, some of which might even be security problems.
Attachment #283746 - Flags: superreview+
Attachment #283746 - Flags: review?(jonas)
Attachment #283746 - Flags: review+
Attachment #283746 - Flags: approval1.9?
Flags: blocking1.9?
Version: unspecified → Trunk

Updated

10 years ago
Attachment #283746 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 9

10 years ago
Created attachment 301951 [details] [diff] [review]
Straightforward fixes. take 4. Updated so it applies.

I took out the aScrollHorizontal change for now. I'll have to run the tool again to remember why I changed it. I'll submit a fix for that in the next batch of fixes.
Attachment #283746 - Attachment is obsolete: true

Updated

10 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
(Assignee)

Comment 10

10 years ago
> >Index: content/xbl/src/nsXBLPrototypeHandler.cpp
> >-  PRBool isXULKey = (mType & NS_HANDLER_TYPE_XUL);
> >-  PRBool isXBLCommand = (mType & NS_HANDLER_TYPE_XBL_COMMAND);
> >+  PRBool isXULKey = !!(mType & NS_HANDLER_TYPE_XUL);
> >+  PRBool isXBLCommand = !!(mType & NS_HANDLER_TYPE_XBL_COMMAND);
> 
> I'm sort of on the fence here what the right thing to do for these are. We
> could do what the above patch does, or we could turn them into PRUint32s, or we
> could detect that these are only used in contexts where we check them for 0
> (|if (isXULKey) {... |) and then ignore them.
> 
> Neither is very appealing.
> 

In theory I could extend my tool to support these special cases that are only
used in if statements. However I think it'd actually cause more trouble if
someone later comes along and decides to use the variables in a different way,
such as passing them to functions.
Keywords: checkin-needed
Yes, we'd want to be continuously be running your tool to see if that happens, which makes that option not very appealing. Since neither option is very good I think we should go with the simple thing which is what the patch does.
Checking in content/base/src/nsContentUtils.cpp;
/cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v  <--  nsContentUtils.cpp
new revision: 1.279; previous revision: 1.278
done
Checking in content/base/src/nsXMLHttpRequest.cpp;
/cvsroot/mozilla/content/base/src/nsXMLHttpRequest.cpp,v  <--  nsXMLHttpRequest.cpp
new revision: 1.217; previous revision: 1.216
done
Checking in content/canvas/src/nsCanvasRenderingContext2D.cpp;
/cvsroot/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp,v  <--  nsCanvasRenderingContext2D.cpp
new revision: 1.115; previous revision: 1.114
done
Checking in content/events/src/nsIMEStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsIMEStateManager.cpp,v  <--  nsIMEStateManager.cpp
new revision: 1.8; previous revision: 1.7
done
Checking in content/html/content/src/nsGenericHTMLElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v  <--  nsGenericHTMLElement.cpp
new revision: 1.751; previous revision: 1.750
done
Checking in content/html/content/src/nsHTMLInputElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLInputElement.cpp,v  <--  nsHTMLInputElement.cpp
new revision: 1.473; previous revision: 1.472
done
Checking in content/xbl/src/nsXBLPrototypeHandler.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp,v  <--  nsXBLPrototypeHandler.cpp
new revision: 1.132; previous revision: 1.131
done
Checking in content/xslt/src/xpath/txRelationalExpr.cpp;
/cvsroot/mozilla/content/xslt/src/xpath/txRelationalExpr.cpp,v  <--  txRelationalExpr.cpp
new revision: 1.34; previous revision: 1.33
done
Checking in content/xslt/src/xslt/txCurrentFunctionCall.cpp;
/cvsroot/mozilla/content/xslt/src/xslt/txCurrentFunctionCall.cpp,v  <--  txCurrentFunctionCall.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in content/xslt/src/xslt/txStylesheetCompiler.cpp;
/cvsroot/mozilla/content/xslt/src/xslt/txStylesheetCompiler.cpp,v  <--  txStylesheetCompiler.cpp
new revision: 1.36; previous revision: 1.35
done
Checking in content/xslt/src/xslt/txXSLTPatterns.cpp;
/cvsroot/mozilla/content/xslt/src/xslt/txXSLTPatterns.cpp,v  <--  txXSLTPatterns.cpp
new revision: 1.34; previous revision: 1.33
done
Checking in content/xul/content/src/nsXULElement.h;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.h,v  <--  nsXULElement.h
new revision: 1.271; previous revision: 1.270
done
Checking in content/xul/templates/src/nsRDFConInstanceTestNode.cpp;
/cvsroot/mozilla/content/xul/templates/src/nsRDFConInstanceTestNode.cpp,v  <--  nsRDFConInstanceTestNode.cpp
new revision: 1.13; previous revision: 1.12
done
Checking in content/xul/templates/src/nsRDFConMemberTestNode.cpp;
/cvsroot/mozilla/content/xul/templates/src/nsRDFConMemberTestNode.cpp,v  <--  nsRDFConMemberTestNode.cpp
new revision: 1.14; previous revision: 1.13
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
You need to log in before you can comment on or make changes to this bug.