| Summary: | Crash in WebCore::UserGestureIndicator::processingUserGesture with WebWorkers | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||||
| Component: | WebCore JavaScript | Assignee: | Dean Jackson <dino> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, ap, cfleizach, commit-queue, dmazzoni, esprehn+autocc, glenn, gyuyoung.kim, japhet, jcraig, jdiggs, kangil.han, kondapallykalyan, mario, samuel_white | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Dean Jackson
2014-10-13 15:25:55 PDT
Created attachment 239753 [details]
Patch
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? > 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. (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. Created attachment 239818 [details]
Patch
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? Seems like this might be failing as a result: fast/dom/remove-body-during-body-replacement2.html 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). Great! Committed r174700: <http://trac.webkit.org/changeset/174700> |