Bug 137676 – Crash in WebCore::UserGestureIndicator::processingUserGesture with WebWorkers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dean Jackson]

Crash in WebCore::UserGestureIndicator::processingUserGesture with WebWorkers

[Dean Jackson]

Created attachment 239753 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 239753 [details]
Patch

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

Can a regression test be added for this?

I'm concerned that this is not right when there are multiple pages served by one process - interacting with one tab doesn't mean that we interacted with another.

> Source/WebCore/loader/FrameLoader.cpp:1862
> +    if (m_frame.isMainFrame())
> +        UserGestureIndicator::resetLastUserGestureTimestamp();

Did you check that this code runs when doing a cached back/forward load?

[Alexey Proskuryakov]

<rdar://problem/15735049>

[Alexey Proskuryakov]

> Can a regression test be added for this?

OK, so it looks like the answer is no, and that's because the crash was already fixed, probably in <http://trac.webkit.org/changeset/152238>, which was then polished in <http://trac.webkit.org/changeset/159198>.

ScriptController::processingUserGesture() and UserGestureIndicator::processingUserGesture() are now very simple functions that can't crash.

I still think that much of the refactoring done in this patch is needed - the code inEventTarget::fireEventListeners() is quite incorrect. Dispatching any arbitrary event shouldn't result in calling resetLastHandledUserGestureTimestamp(), as most events are not user gestures.

[Dean Jackson]

(In reply to comment #2)

> > Source/WebCore/loader/FrameLoader.cpp:1862
> > +    if (m_frame.isMainFrame())
> > +        UserGestureIndicator::resetLastUserGestureTimestamp();
> 
> Did you check that this code runs when doing a cached back/forward load?

In the refactored patch (about to be posted) we don't actually care about this any more.

[Dean Jackson]

Created attachment 239818 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 239818 [details]
Patch

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

Looks good to me. Please wait to EWS to agree :)

> Source/WebCore/dom/UserGestureIndicator.h:46
> +    WEBCORE_EXPORT explicit UserGestureIndicator(ProcessingUserGestureState, Document* = nullptr);

Maybe add a comment here, explaining that latest user event timestamp is updated in the document if provided?

[Dean Jackson]

Seems like this might be failing as a result: fast/dom/remove-body-during-body-replacement2.html

[Alexey Proskuryakov]

Actually, fast/dom/remove-body-during-body-replacement2.html regressed with r174637. And fast/hidpi/image-srcset-relative-svg-canvas.html has been very flaky on EWS recently (but not on regular bots).

[Dean Jackson]

Great!

[Dean Jackson]

Committed r174700: <http://trac.webkit.org/changeset/174700>