Bug 34156 – Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Roland Steiner]

Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp)

[Roland Steiner]

Created attachment 47391 [details]
Patch

[Roland Steiner]

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

[Darin Adler]

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.

[Roland Steiner]

Created attachment 47478 [details]
patch - simplified expression

[Darin Adler]

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.

[Roland Steiner]

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.

[Roland Steiner]

(forgot: patch was committed in rev. 53885)

[Darin Adler]

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