Bug 137676 - Crash in WebCore::UserGestureIndicator::processingUserGesture with WebWorkers
Summary: Crash in WebCore::UserGestureIndicator::processingUserGesture with WebWorkers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-10-13 15:25 PDT by Dean Jackson
Modified: 2014-10-14 14:02 PDT (History)
16 users (show)

See Also:


Attachments
Patch (10.95 KB, patch)
2014-10-13 15:39 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (14.06 KB, patch)
2014-10-14 12:43 PDT, Dean Jackson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2014-10-13 15:25:55 PDT
Crash in WebCore::UserGestureIndicator::processingUserGesture with WebWorkers
Comment 1 Dean Jackson 2014-10-13 15:39:29 PDT
Created attachment 239753 [details]
Patch
Comment 2 Alexey Proskuryakov 2014-10-13 15:48:03 PDT
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?
Comment 3 Alexey Proskuryakov 2014-10-13 23:10:44 PDT
<rdar://problem/15735049>
Comment 4 Alexey Proskuryakov 2014-10-13 23:54:00 PDT
> 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.
Comment 5 Dean Jackson 2014-10-14 12:41:06 PDT
(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.
Comment 6 Dean Jackson 2014-10-14 12:43:51 PDT
Created attachment 239818 [details]
Patch
Comment 7 Alexey Proskuryakov 2014-10-14 12:53:47 PDT
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?
Comment 8 Dean Jackson 2014-10-14 13:36:13 PDT
Seems like this might be failing as a result: fast/dom/remove-body-during-body-replacement2.html
Comment 9 Alexey Proskuryakov 2014-10-14 13:45:29 PDT
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).
Comment 10 Dean Jackson 2014-10-14 13:58:31 PDT
Great!
Comment 11 Dean Jackson 2014-10-14 14:02:24 PDT
Committed r174700: <http://trac.webkit.org/changeset/174700>