Bug 179651 - iOS: Enable release asserts in updateStyleIfNeeded() and updateLayout() for WebKit2
Summary: iOS: Enable release asserts in updateStyleIfNeeded() and updateLayout() for W...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-13 19:40 PST by Ryosuke Niwa
Modified: 2017-11-17 19:30 PST (History)
18 users (show)

See Also:


Attachments
Re-enables the asserts (3.02 KB, patch)
2017-11-13 19:47 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (3.12 KB, patch)
2017-11-13 21:17 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-11-13 19:40:00 PST
https://trac.webkit.org/r224604 disabled these release asserts in iOS due to a bug in UIKit for WebKit1.
Re-enable these release assertions in iOS WebKit2 port.
Comment 1 Radar WebKit Bug Importer 2017-11-13 19:40:30 PST
<rdar://problem/35523034>
Comment 2 Ryosuke Niwa 2017-11-13 19:47:12 PST
Created attachment 326841 [details]
Re-enables the asserts
Comment 3 Antti Koivisto 2017-11-13 19:55:05 PST
Comment on attachment 326841 [details]
Re-enables the asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=326841&action=review

r=me

> Source/WebCore/dom/Document.cpp:1931
> +    bool usingWebThread = WebThreadIsEnabled();

:(
Comment 4 Simon Fraser (smfr) 2017-11-13 20:11:15 PST
Comment on attachment 326841 [details]
Re-enables the asserts

View in context: https://bugs.webkit.org/attachment.cgi?id=326841&action=review

> Source/WebCore/dom/Document.cpp:1935
> +    return NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()) || usingWebThread;

This is pretty confusing to read. This function is about it being safe to update style or do layout, but to a lay reader, what does that have to do with event dispatch or frame flattening?

Also NoEventDispatchAssertion::InMainThread::isEventAllowed(): so there's the "no event" negative vs. the "is event allowed", then there's InMainThread but the later usingWebThread check

There's altogether too much discordance here. And, as Darin recommends, an assertion should only check one thing, not a list of things (because then you can't tell which failed).
Comment 5 Ryosuke Niwa 2017-11-13 20:41:32 PST
(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 326841 [details]
> Re-enables the asserts
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326841&action=review
> 
> > Source/WebCore/dom/Document.cpp:1935
> > +    return NoEventDispatchAssertion::InMainThread::isEventAllowed() || (frameView && frameView->isInChildFrameWithFrameFlattening()) || usingWebThread;
> 
> This is pretty confusing to read. This function is about it being safe to
> update style or do layout, but to a lay reader, what does that have to do
> with event dispatch or frame flattening?

Indeed. I'd refactor it to as:

bool isSafeToExecuteScript = NoEventDispatchAssertion::InMainThread::isEventAllowed();
bool isInFrameFlattening = frameView && frameView->isInChildFrameWithFrameFlattening();
return isSafeToExecuteScript || isInFrameFlattening || usingWebThread;

> Also NoEventDispatchAssertion::InMainThread::isEventAllowed(): so there's
> the "no event" negative vs. the "is event allowed", then there's
> InMainThread but the later usingWebThread check

I agree we should rename that but such a renaming / refactoring is outside the scope of this patch.

> There's altogether too much discordance here. And, as Darin recommends, an
> assertion should only check one thing, not a list of things (because then
> you can't tell which failed).

The problem is that we can't do it until frame flattening stops doing a nested layout or iOS WebKit bug (<rdar://problem/35522719>) is fixed.

An alternative is to do something like this:
if (!isInFrameFlattening && !usingWebThread)
    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::InMainThread::isEventAllowed());

I'm not certain if that code is better.
Comment 6 Ryosuke Niwa 2017-11-13 21:17:16 PST
Created attachment 326851 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2017-11-13 22:02:36 PST
The commit-queue encountered the following flaky tests while processing attachment 326851 [details]:

svg/animations/svgtransform-animation-1.html bug 179354 (authors: ap@webkit.org, arv@chromium.org, krit@webkit.org, mark.lam@apple.com, and zimmermann@kde.org)
The commit-queue is continuing to process your patch.
Comment 8 WebKit Commit Bot 2017-11-13 22:03:11 PST
Comment on attachment 326851 [details]
Patch for landing

Clearing flags on attachment: 326851

Committed r224803: <https://trac.webkit.org/changeset/224803>
Comment 9 WebKit Commit Bot 2017-11-13 22:03:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Simon Fraser (smfr) 2017-11-13 22:34:56 PST
> An alternative is to do something like this:
> if (!isInFrameFlattening && !usingWebThread)
>    
> RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::
> InMainThread::isEventAllowed());
> 
> I'm not certain if that code is better.

Maybe better?

RELEASE_ASSERT_IMPLIES_WITH_SECURITY_IMPLICATION(!isInFrameFlattening && !usingWebThread, NoEventDispatchAssertion::InMainThread::isEventAllowed());

if we make that macro.

What's the difference between RELEASE_ASSERT_IMPLIES_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT?
Comment 11 David Kilzer (:ddkilzer) 2017-11-13 23:39:59 PST
(In reply to Simon Fraser (smfr) from comment #10)
> > An alternative is to do something like this:
> > if (!isInFrameFlattening && !usingWebThread)
> >    
> > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(NoEventDispatchAssertion::
> > InMainThread::isEventAllowed());
> > 
> > I'm not certain if that code is better.
> 
> Maybe better?
> 
> RELEASE_ASSERT_IMPLIES_WITH_SECURITY_IMPLICATION(!isInFrameFlattening &&
> !usingWebThread, NoEventDispatchAssertion::InMainThread::isEventAllowed());
> 
> if we make that macro.

I'm not sure what that macro does or what it's based on, but maybe I'm missing something.

> What's the difference between
> RELEASE_ASSERT_IMPLIES_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT?

Did you mean RELEASE_ASSERT_WITH_SECURITY_IMPLICATION?

In Release builds, they're the same.  In Debug builds, they'll have a different exception code (0xfbadbeef for security, 0xbbadbeef for non-security).

See also:  <https://bugs.webkit.org/show_bug.cgi?id=178269#c0>
Comment 12 Alexey Proskuryakov 2017-11-17 10:14:15 PST
> The commit-queue encountered the following flaky tests while processing attachment 326851 [details]:

> svg/animations/svgtransform-animation-1.html bug 179354 (authors: ap@webkit.org, arv@chromium.org, krit@webkit.org, mark.lam@apple.com, and zimmermann@kde.org)

Was this a legitimate issue discovered by EWS?
Comment 13 Ryosuke Niwa 2017-11-17 19:30:31 PST
(In reply to Alexey Proskuryakov from comment #12)
> > The commit-queue encountered the following flaky tests while processing attachment 326851 [details]:
> 
> > svg/animations/svgtransform-animation-1.html bug 179354 (authors: ap@webkit.org, arv@chromium.org, krit@webkit.org, mark.lam@apple.com, and zimmermann@kde.org)
> 
> Was this a legitimate issue discovered by EWS?

Oh, I didn't even see it but it is https://bugs.webkit.org/show_bug.cgi?id=179306.