398433 - PRBool misuse bugs in content/

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[(dormant account)]

Attachment: Straightforward fixes

This is a manually corrected version of the prcheck-produced patch.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I prefer !!(expr) syntax, but Jonas may have different opinion.

Comment 2

[(dormant account)]

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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 4

[(dormant account)]

Attachment: 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?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[(dormant account)]

Attachment: 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()

Comment 7

[(dormant account)]

Jonas: ping.
Could you please review the patch.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[(dormant account)]

Attachment: 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.

Comment 10

[(dormant account)]

> >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.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 12

[Reed Loden [:reed]]

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