Bug 179651

<rdar://problem/35523034>

Created attachment 326841 [details] Re-enables the asserts

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

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

Created attachment 326851 [details] Patch for landing

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 on attachment 326851 [details] Patch for landing Clearing flags on attachment: 326851 Committed r224803: <https://trac.webkit.org/changeset/224803>

All reviewed patches have been landed. Closing bug.

> 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?

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

> 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?

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