Closed Bug 1044074 Opened 10 years ago Closed 6 years ago

Nested event loops do not suspend scroll events

Categories

(DevTools :: Debugger, defect)

Product:
DevTools
Component:
Debugger
Version:
31 Branch
Platform:
x86_64
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox66 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: rigdern, Assigned: bhackett1024)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [devtools-platform])

Attachments

(5 files, 2 obsolete files)

scrollRepro.html
904 bytes, text/html
Details
patch
10.70 KB, patch
smaug
: review+
Details | Diff | Splinter Review
followup
2.67 KB, patch
smaug
: review+
Details | Diff | Splinter Review
patch
8.97 KB, patch
Details | Diff | Splinter Review
scroll event test
3.15 KB, patch
Details | Diff | Splinter Review
Attached file scrollRepro.html —
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.125 Safari/537.36 Steps to reproduce: 1. Open scrollRepro.html (attached) 2. Open the debugger 3. Click the "Repro" button 4. When the debugger breaks, resume the script. Actual results: Firefox shows the following in the console: - before debugger - handleScroll - afterDebugger Expected results: Firefox should show the following in the console: - before debugger - afterDebugger - handleScroll
This might be a dupe of bug 801304. This bug is about scroll events while the other one is about XMLHttpRequests.
Component: Untriaged → Developer Tools: Debugger
See Also: → 801304
Boris, when we suppress timeouts, can scroll events still be delivered somehow? Based on what I read in bug 810304, this is apparently possible for XHR, so I'm wondering whether this bug has the same underlying cause or not.
Flags: needinfo?(bzbarsky)
> when we suppress timeouts, can scroll events still be delivered somehow? I would guess yes, but Olli would know for sure.
Flags: needinfo?(bzbarsky)
Summary: Debugger breakpoint breaks run-to-completion for scroll events → Nested event loops do not suspend scroll events
Blocks: dbg-r2c
Blocks: 1039795
(In reply to Boris Zbarsky [:bz] from comment #3) > > when we suppress timeouts, can scroll events still be delivered somehow? > > I would guess yes, but Olli would know for sure. Olli, do you know the answer to the question in comment 2?
Flags: needinfo?(bugs)
scroll events are triggered based on refreshdriver/painting stuff, so we don't suppress them atm.
Flags: needinfo?(bugs)
Whiteboard: [devtools-platform]
Another test case: <!DOCTYPE html> <html> <body> <div id="count"></div> <div style="overflow:scroll"> <div style="height: 5000px"></div> </div> <script> var i = 0; var count = document.getElementById("count"); window.onscroll = function () { count.textContent = i++; }; debugger; </script> </body> </html> Open in the debugger, pause at the debugger statement, then scroll up and down and observe the counter at the top of the page updating.
Product: Firefox → DevTools
Attached patch patch (obsolete) — — Splinter Review
This patch suppresses event handling for resizes and scrolls that are triggered by the refresh driver while the associated window's document has events suppressed (including when the debugger has paused). These events are just dropped: after the debugger unpauses they will not fire. This seems preferable to allowing the events to fire while the debugger is paused and break run-to-completion, and there doesn't seem to be an existing way to delay these events so that they happen later.
Assignee: nobody → bhackett1024
I like the idea. I think it makes sense to just drop these events - they're not like other JS invocations like worker messages or XHRs, which would really confuse the page content if they were dropped. But it would be nice if the debuggee, upon being resumed, could get a single 'catch-up' scroll or event, so that it doesn't come out of the debugger pause with a confused notion of where it is, or how large the window is. That is, the platform doesn't guarantee to send any particular number of scroll events, but when scrolling happens, it does promise to send at least one.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch patch (obsolete) — — Splinter Review
This implements the idea from comment 9 for scroll events (bug 1513720 takes care of resize events). Scroll events are not dispatched to document content as long as event handling is suppressed, and a single one is dispatched afterwards for each frame that was scrolled while paused.
Attachment #9030278 - Attachment is obsolete: true
Attachment #9030935 - Flags: review?(bugs)
You'll need something similar for animation events and animation frame callbacks and what not, right?
(In reply to Olli Pettay [:smaug] (high review load) from comment #11) > You'll need something similar for animation events and animation frame > callbacks and what not, right? Yeah, though for now we're just picking off different sources of run-to-completion bugs, one-by-one. The triggering mechanisms and desired behavior vary between each kind of event, which makes it hard to handle them in a uniform way. I filed bug 1513727 for animation events. AFAICT from experiments and reading (nsIDocument::UpdateFrameRequestCallbackSchedulingState), animation frame callbacks are already ignored when the document has suppressed event handling. With this and bug 1513720 (for resize events), we have bugs and/or fixes for all the different events I can find that are triggered via nsRefreshDriver::Tick --- scroll, resize, animation, and frame request callbacks. Are there others missing from this list?
I need to think a bit how this all affects to non-devtools event suppression usage. Event suppression after all wasn't designed for devtools. (but my gut feeling is that this will improve event suppression behavior everywhere)
Comment on attachment 9030935 [details] [diff] [review] patch This keeps refresh driver ticking while event handling is suppressed - so, while sync XHR or debugger is active. Don't really like that approach. Perhaps we need something generic in RefreshDriver to deal with things which shouldn't run when event handling is suppressed.
Attachment #9030935 - Flags: review?(bugs) → review-
Attached patch patch — — Splinter Review
This patch updates the refresh driver so that it separately keeps track of scroll events associated with a document that had events suppressed at the time the event was added to the driver. These events are triggered like other scroll events, but they do not contribute to the tests for whether the driver has any observers, so that the driver does not keep ticking while a scroll event is being suppressed. This patch also takes care of resize events using the same technique, since there is overlap in the implementations. I'll try to write a test for these changes before landing.
Attachment #9032034 - Flags: review?(bugs)
Attachment #9030935 - Flags: review-
Attachment #9032034 - Flags: review?(bugs) → review+
Attachment #9030935 - Attachment is obsolete: true
Comment on attachment 9032034 [details] [diff] [review] patch Review of attachment 9032034 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this is the kind of machinery it seems like we would need. This looks great.
Attached patch followup — — Splinter Review
Sorry about the review churn, but I noticed a problem with the last patch: calling RunDelayedEventsSoon() after a document unsuppresses events won't necessarily do anything, as the events stay in their delayed arrays and the refresh driver will check for any observers before processing events and stop if there aren't any in the normal arrays. This patch fixes the problem by moving the contents of the delayed arrays into the normal arrays in RunDelayedEventsSoon(). This has the nice effect of removing the need to check the delayed events during event processing, which both reduces the total size of the diff and ensures a stronger property for the delayed events: they are guaranteed to be associated with a document which is suppressing events.
Attachment #9032235 - Flags: review?(bugs)
Attached patch patch — — Splinter Review
For reference, the updated complete patch.
Attachment #9032235 - Flags: review?(bugs) → review+
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/91fb09d21018 Don't run resize or scroll events while documents have events suppressed, r=smaug.
The above patch doesn't have a test, but I'm still working on writing one. It's a little complicated to get right due to issues like bug 1515302.
https://hg.mozilla.org/mozilla-central/rev/91fb09d21018
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Blocks: 1515778
Test that scroll event handlers don't break run-to-completion. I'll land this when it looks good on try. There doesn't seem to be a simple way to do this for resize events.
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5327afef99b0 Add test that scroll event handlers do not break run-to-completion, no_r.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: