Nested event loops do not suspend scroll events

RESOLVED FIXED in Firefox 66

Status

defect
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: rigdern, Assigned: bhackett)

Tracking

(Blocks 2 bugs)

31 Branch
Firefox 66
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

(Whiteboard: [devtools-platform])

Attachments

(5 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
Posted 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
Reporter

Comment 1

5 years ago
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

Updated

5 years ago
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
(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.
Duplicate of this bug: 1312592

Updated

Last year
Product: Firefox → DevTools
Assignee

Comment 8

7 months ago
Posted 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

Comment 9

7 months ago
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
Assignee

Comment 10

6 months ago
Posted 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?
Assignee

Comment 12

6 months ago
(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-
Assignee

Comment 15

6 months ago
Posted patch patchSplinter 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)
Assignee

Updated

6 months ago
Attachment #9030935 - Flags: review-

Updated

6 months ago
Attachment #9032034 - Flags: review?(bugs) → review+
Assignee

Updated

6 months ago
Attachment #9030935 - Attachment is obsolete: true

Comment 16

6 months ago
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.
Assignee

Comment 17

6 months ago
Posted patch followupSplinter 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)
Assignee

Comment 18

6 months ago
Posted patch patchSplinter Review
For reference, the updated complete patch.

Updated

6 months ago
Attachment #9032235 - Flags: review?(bugs) → review+

Comment 19

6 months ago
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.
Assignee

Comment 20

6 months ago
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.

Comment 21

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91fb09d21018
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Assignee

Updated

6 months ago
Duplicate of this bug: 1513720
Assignee

Comment 23

6 months ago
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.

Comment 24

6 months ago
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.
Duplicate of this bug: 1515778
You need to log in before you can comment on or make changes to this bug.