Bug 105348 – XMLHttpRequestProgressEventThrottle::resume() always schedules timer even when unnecessary

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Yong Li]

XMLHttpRequestProgressEventThrottle::resume() currently always books a timer even there is no deferred event to handle.

It should just clear m_deferEvents and return in this case.

[Yong Li]

The patch will be as easy as

::resume()
{
 ...

+ if (no deferred event to dispatch) {
+   clear flag and return;
+ }
 ...

No time today. will post a patch later

[Yong Li]

Created attachment 187607 [details]
the patch

[Alexey Proskuryakov]

Comment on attachment 187607 [details]
the patch

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

Looks right to me.

> Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp:202
> +        // There is no deferred event. Let's clear the flag now.

I don't think that this comment is useful. It just describes what the code does.

Please remove the comment before landing.

[Yong Li]

Created attachment 187679 [details]
remove the comment

Thanks Alexey

[Allan Sandfeld Jensen]

Comment on attachment 187679 [details]
remove the comment

Even if no events are deferred yet, they may be before events are starting to be dispatched. In any case no event should be dispatched if none are there at the time.

[Allan Sandfeld Jensen]

(In reply to comment #0)
> XMLHttpRequestProgressEventThrottle::resume() currently always books a timer even there is no deferred event to handle.
> 

Yes, an instant timer. Please read the comments for it before assuming that is wrong.

[Yong Li]

(In reply to comment #5)
> (From update of attachment 187679 [details])
> Even if no events are deferred yet, they may be before events are starting to be dispatched. In any case no event should be dispatched if none are there at the time.


I'm not sure I get you. But leaving the flag on will automatically defer incoming events, even when not necessary. Deferring events is only needed in special cases.

[Yong Li]

(In reply to comment #6)
> (In reply to comment #0)
> > XMLHttpRequestProgressEventThrottle::resume() currently always books a timer even there is no deferred event to handle.
> > 
> 
> Yes, an instant timer. Please read the comments for it before assuming that is wrong.

Why should we start the timer if it is not necessary?

[Allan Sandfeld Jensen]

The patch may be safe. The reason I paniced and put cq- on it is just to get time to review it tomorrow. The code here was the subject of a large number of race-conditions, and your patch also conflicts a little with comment below it:
// m_deferEvents is kept true until all deferred events have been dispatched.

The problem with resume is that a number of active DOM objects can be resumed at the same time, and the request here can both be cancelled or even suspended again before it has a chance to fully react to resume.

[Allan Sandfeld Jensen]

Comment on attachment 187679 [details]
remove the comment

Okay, I have had a few minutes to check it, and I can't think of a race-condition that could go wrong, yet, but since ap already approved it. I will not hold it back.

[Alexey Proskuryakov]

> your patch also conflicts a little with comment below it:
> // m_deferEvents is kept true until all deferred events have been dispatched.

I don't think that there is a conflict there. With this change, m_deferEvents is set to false only when there are no deferred events.

Note that this comment disagrees with what XMLHttpRequestProgressEventThrottle::dispatchDeferredEvents() actually does. It sets m_deferEvents to false immediately, and even notes that there may be leftover events remaining in m_deferredEvents.

So I think that this patch is correct, however the code needs another look.

[Allan Sandfeld Jensen]

(In reply to comment #11)
> > your patch also conflicts a little with comment below it:
> > // m_deferEvents is kept true until all deferred events have been dispatched.
> 
> I don't think that there is a conflict there. With this change, m_deferEvents is set to false only when there are no deferred events.
> 
> Note that this comment disagrees with what XMLHttpRequestProgressEventThrottle::dispatchDeferredEvents() actually does. It sets m_deferEvents to false immediately, and even notes that there may be leftover events remaining in m_deferredEvents.
> 
> So I think that this patch is correct, however the code needs another look.

The point in this case is that m_deferEvents means that resume has been called, but dispatchDeferredEvents() has not. This is a half-resumed state that can be cancelled in case something else causes a suspend before dispatchDeferredEvents() has a chance to run.

[WebKit Review Bot]

Comment on attachment 187679 [details]
remove the comment

Clearing flags on attachment: 187679

Committed r142538: <http://trac.webkit.org/changeset/142538>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yong Li]

Thanks Alan for double checking.