Bug 39280 - Wrong ternary in CSSStyleSelector::SelectorChecker::checkOneSelector
Summary: Wrong ternary in CSSStyleSelector::SelectorChecker::checkOneSelector
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Trivial
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-18 03:29 PDT by Yoshiki Hayashi
Modified: 2010-05-18 20:42 PDT (History)
3 users (show)

See Also:


Attachments
Proposed Patch (8.75 KB, patch)
2010-05-18 03:36 PDT, Yoshiki Hayashi
mitz: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yoshiki Hayashi 2010-05-18 03:29:03 PDT
In CSSStyleSelector::SelectorChecker::checkOneSelector and checkSelector in WebCore/css/CSSStyleSelector.cpp, there are multiple occurrence of ternary that looks like:
RenderStyle* parentStyle = elementStyle ? elementParentStyle : e->parentNode()->renderStyle();
where it should look like:
RenderStyle* parentStyle = elementParentStyle ? elementParentStyle : e->parentNode()->renderStyle();

If I'm reading the code correctly, elementStyle and elementParentStyle are either both NULL or both non-NULL so there isn't any visible problem because of this code.  However, I think it's worth fixing so I'm going to attach a patch to fix it.

Thanks,
Comment 1 Yoshiki Hayashi 2010-05-18 03:36:06 PDT
Created attachment 56353 [details]
Proposed Patch
Comment 2 mitz 2010-05-18 08:09:33 PDT
Actually, I think this was done on purpose. I would add an assertion that elementParentstyle is null iff elementstyle is null, but not change this code, as it is slightly more robust.
Comment 3 Darin Adler 2010-05-18 09:02:28 PDT
Comment on attachment 56353 [details]
Proposed Patch

You couldn't find any case where this led to a null-dereference? If you could find a case then it would be best to add a test case to demonstrate the problem.

r=me
Comment 4 mitz 2010-05-18 09:51:31 PDT
Comment on attachment 56353 [details]
Proposed Patch

I am sorry. I should have set to r- based on my comment. Please don’t make this change.
Comment 5 mitz 2010-05-18 09:52:15 PDT
(In reply to comment #3)
> (From update of attachment 56353 [details])
> You couldn't find any case where this led to a null-dereference?

parentStyle appears to be null-checked everywhere.
Comment 6 Yoshiki Hayashi 2010-05-18 20:42:43 PDT
Thank you Dan and Darin for prompt review.  Since the code is intentional according to Dan, I'm just going to close this issue.

Thanks,