Bug 34156 - Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp)
: Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp)
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-01-26 00:56 PST by
Modified: 2010-01-29 13:42 PST (History)


Attachments
Patch (1.38 KB, patch)
2010-01-26 00:59 PST, Roland Steiner
no flags Review Patch | Details | Formatted Diff | Diff
patch - simplified expression (1.17 KB, patch)
2010-01-26 18:01 PST, Roland Steiner
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-01-26 00:56:28 PST
Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp)
------- Comment #1 From 2010-01-26 00:59:28 PST -------
Created an attachment (id=47391) [details]
Patch
------- Comment #2 From 2010-01-26 01:01:03 PST -------
Could potentially lead to a crash if applied to an attribute named "blockquote",
or an incorrect result if applied to a different element that has an attribute "cite".
------- Comment #3 From 2010-01-26 08:18:36 PST -------
(From update of attachment 47391 [details])
> +        Could potentially lead to a crash if applied to an attribute named "blockquote",

That's incorrect. The Node::hasTagName will only return true for an Element. You can look at the implementation to double-check this.

> +        or an incorrect result if applied to a different element that has an attribute "cite".

I don't understand this claim.

> +        No new tests. (minor code change)

If the "cite" case existed, you could probably construct a test case for it.

> -    if (!node || (!node->isElementNode() && !node->hasTagName(blockquoteTag)))
> +    if (!node || !node->isElementNode() || !node->hasTagName(blockquoteTag))
>          return false;

The isElementNode check is entirely unnecessary and can be removed. That's probably the best fix.
------- Comment #4 From 2010-01-26 18:01:52 PST -------
Created an attachment (id=47478) [details]
patch - simplified expression
------- Comment #5 From 2010-01-27 12:21:29 PST -------
It’s too late for this, I guess, but I realized that there must be a real bug here. There *must* have been some way to get this code to run for non-blockquote elements just by adding a cite attribute to them, and this in turn must have done the wrong thing in the editing operation. If we dig into that a bit I think we could reproduce incorrect behavior, although probably not a crash.

So a test case would have been possible and should have been developed.
------- Comment #6 From 2010-01-27 21:09:24 PST -------
Yes (btw, that was what I was referring to in my remark, was just poorly worded). 

I'll probably continue to be working on editing anyway, so I'll keep it in the back of my head and try to come up with (or include it in) a good test case.
------- Comment #7 From 2010-01-27 21:11:09 PST -------
(forgot: patch was committed in rev. 53885)
------- Comment #8 From 2010-01-29 13:42:39 PST -------
(In reply to comment #6)
> Yes (btw, that was what I was referring to in my remark, was just poorly
> worded).

Makes sense. I eventually figured that out.