Fix clang static analyzer warnings: Branch condition evaluates to a garbage value.
<rdar://problem/41397137>
Created attachment 343432 [details] Patch v1
Comment on attachment 343432 [details] Patch v1 Some of them are clearly meant to be optional<>, like the case of isShorthand, if the property is invalid (the early return case) isShorthand should have no value. Also with changing the method signature you could discover other callers with garbage value (emphasisMarkExistsAndIsAbove -> called from paintMarkedTextForeground() and emphasisMarkAbove is not set).
(In reply to zalan from comment #3) > Comment on attachment 343432 [details] > Patch v1 > > Some of them are clearly meant to be optional<>, like the case of > isShorthand, if the property is invalid (the early return case) isShorthand > should have no value. Also with changing the method signature you could > discover other callers with garbage value (emphasisMarkExistsAndIsAbove -> > called from paintMarkedTextForeground() and emphasisMarkAbove is not set). Changing the method signature to use std::optional<bool> totally caught other cases of uninitialized stack variables. Thanks!! The only thing I'm not sure about is which style is preferred: if (isShorthand.has_value() && isShorthand.value()) Or: if (isShorthand.value_or(false)) The second one is more concise, but I find I have to think about its correctness much more than the longer version.
Created attachment 343483 [details] Patch v2
(In reply to David Kilzer (:ddkilzer) from comment #4) > (In reply to zalan from comment #3) > > Comment on attachment 343432 [details] > > Patch v1 > > > > Some of them are clearly meant to be optional<>, like the case of > > isShorthand, if the property is invalid (the early return case) isShorthand > > should have no value. Also with changing the method signature you could > > discover other callers with garbage value (emphasisMarkExistsAndIsAbove -> > > called from paintMarkedTextForeground() and emphasisMarkAbove is not set). > > Changing the method signature to use std::optional<bool> totally caught > other cases of uninitialized stack variables. Thanks!! > > The only thing I'm not sure about is which style is preferred: > > if (isShorthand.has_value() && isShorthand.value()) > > Or: > > if (isShorthand.value_or(false)) > > The second one is more concise, but I find I have to think about its > correctness much more than the longer version. I usually do if (isShorthand && *isShorthand) but not sure if that's the preferred way of doing it.
Comment on attachment 343483 [details] Patch v2 Attachment 343483 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8324535 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html
Created attachment 343487 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Build Bot from comment #7) > Comment on attachment 343483 [details] > Patch v2 > > Attachment 343483 [details] did not pass win-ews (win): > Output: https://webkit-queues.webkit.org/results/8324535 > > New failing tests: > http/tests/security/video-poster-cross-origin-crash2.html These tests apparently aren't crashing on Windows bots (although both Release/Debug Windows builds are currently failing.) <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fpreload%2Fonload_event.html%2C%20http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2FuserAgentShadowDOM%2Fallow-video.html%2C%20http%2Ftests%2Fsecurity%2Fvideo-poster-cross-origin-crash2.html> No clue how investigate these failures because there aren't any crash logs in the results archive.
(In reply to David Kilzer (:ddkilzer) from comment #9) > (In reply to Build Bot from comment #7) > > Comment on attachment 343483 [details] > > Patch v2 > > > > Attachment 343483 [details] did not pass win-ews (win): > > Output: https://webkit-queues.webkit.org/results/8324535 > > > > New failing tests: > > http/tests/security/video-poster-cross-origin-crash2.html > > These tests apparently aren't crashing on Windows bots (although both > Release/Debug Windows builds are currently failing.) > This is probably a dupe of https://bugs.webkit.org/show_bug.cgi?id=179297, and is unrelated to this patch. > <https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#tests=http%2Ftests%2Fpreload%2Fonload_event. > html%2C%20http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2FuserAgentShadowDO > M%2Fallow-video.html%2C%20http%2Ftests%2Fsecurity%2Fvideo-poster-cross- > origin-crash2.html> > > No clue how investigate these failures because there aren't any crash logs > in the results archive.
Created attachment 343545 [details] Patch v3
(In reply to zalan from comment #6) > I usually do > if (isShorthand && *isShorthand) > but not sure if that's the preferred way of doing it. After sleeping on it, I do like this much better. It's very concise, and once I know the variable is a std::optional<>, it's easy to recognize the pattern.
Comment on attachment 343545 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=343545&action=review > Source/WebCore/rendering/InlineFlowBox.cpp:1628 > - if (childLineStyle.textEmphasisMark() != TextEmphasisMark::None && !emphasisMarkIsAbove) { > + if (childLineStyle.textEmphasisMark() != TextEmphasisMark::None && emphasisMarkIsAbove && !*emphasisMarkIsAbove) { Make sure to double-check this one. I wasn't sure if it should be ("unknown or not above"): ... && (!emphasisMarkIsAbove || !*emphasisMarkIsAbove)) { Versus ("known and not above"): ... && emphasisMarkIsAbove && !*emphasisMarkIsAbove) {
Comment on attachment 343545 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=343545&action=review > Source/WebCore/rendering/InlineTextBox.cpp:361 > -bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, bool& above) const > +bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, std::optional<bool>& above) const Here is a simpler approach. I think this function can be split into two functions: bool emphasisMarkExists(const RenderStyle& style) bool emphasisMarkIsAbove(const RenderStyle& style) I checked the code and I found all the callers to this function use the boolean 'above' only if emphasisMarkExistsAndIsAbove() returns true. So I do not think there a need to have the two functions combined into one function. > Source/WebCore/rendering/InlineTextBox.cpp:1008 > + std::optional<bool> emphasisMarkAbove; > bool hasTextEmphasis = emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkAbove); > const AtomicString& emphasisMark = hasTextEmphasis ? lineStyle.textEmphasisMarkString() : nullAtom(); > if (!emphasisMark.isEmpty()) > - emphasisMarkOffset = emphasisMarkAbove ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark); > + emphasisMarkOffset = (emphasisMarkAbove && *emphasisMarkAbove) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark); This can be done using two functions like this: const AtomicString& emphasisMark = emphasisMarkExists(lineStyle) ? lineStyle.textEmphasisMarkString() : nullAtom(); if (!emphasisMark.isEmpty()) emphasisMarkOffset = emphasisMarkIsAbove(lineStyle) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
Comment on attachment 343545 [details] Patch v3 Attachment 343545 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8348383 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html
Created attachment 343621 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Said Abou-Hallawa from comment #14) > Comment on attachment 343545 [details] > Patch v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343545&action=review > > > Source/WebCore/rendering/InlineTextBox.cpp:361 > > -bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, bool& above) const > > +bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, std::optional<bool>& above) const > > Here is a simpler approach. I think this function can be split into two > functions: > > bool emphasisMarkExists(const RenderStyle& style) > bool emphasisMarkIsAbove(const RenderStyle& style) > > I checked the code and I found all the callers to this function use the > boolean 'above' only if emphasisMarkExistsAndIsAbove() returns true. So I do > not think there a need to have the two functions combined into one function. > > > Source/WebCore/rendering/InlineTextBox.cpp:1008 > > + std::optional<bool> emphasisMarkAbove; > > bool hasTextEmphasis = emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkAbove); > > const AtomicString& emphasisMark = hasTextEmphasis ? lineStyle.textEmphasisMarkString() : nullAtom(); > > if (!emphasisMark.isEmpty()) > > - emphasisMarkOffset = emphasisMarkAbove ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark); > > + emphasisMarkOffset = (emphasisMarkAbove && *emphasisMarkAbove) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark); > > This can be done using two functions like this: > > const AtomicString& emphasisMark = emphasisMarkExists(lineStyle) ? > lineStyle.textEmphasisMarkString() : nullAtom(); > if (!emphasisMark.isEmpty()) > emphasisMarkOffset = emphasisMarkIsAbove(lineStyle) ? > -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : > font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark); I think this should be addressed in a separate patch.
Comment on attachment 343545 [details] Patch v3 Clearing flags on attachment: 343545 Committed r233267: <https://trac.webkit.org/changeset/233267>
All reviewed patches have been landed. Closing bug.