Bug 34156 - Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp)
: Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp)
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-26 00:56 PST by Roland Steiner
Modified: 2010-01-29 13:42 PST (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 2010-01-26 00:56:28 PST
Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp)
Comment 1 Roland Steiner 2010-01-26 00:59:28 PST
Created attachment 47391 [details]
Patch
Comment 2 Roland Steiner 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 Darin Adler 2010-01-26 08:18:36 PST
Comment on attachment 47391 [details]
Patch

> +        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 Roland Steiner 2010-01-26 18:01:52 PST
Created attachment 47478 [details]
patch - simplified expression
Comment 5 Darin Adler 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 Roland Steiner 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 Roland Steiner 2010-01-27 21:11:09 PST
(forgot: patch was committed in rev. 53885)
Comment 8 Darin Adler 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.