| Summary: | Incorrect boolean expression in isMailBlockquote() (WebCore/htmlediting.cpp) | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Roland Steiner <rolandsteiner> | ||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | darin | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Roland Steiner
2010-01-26 00:56:28 PST
Created attachment 47391 [details]
Patch
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 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. Created attachment 47478 [details]
patch - simplified expression
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. 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. (forgot: patch was committed in rev. 53885) (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. |