Bug 105348

Summary: XMLHttpRequestProgressEventThrottle::resume() always schedules timer even when unnecessary
Product: WebKit Reporter: Yong Li <yong.li.webkit>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, ap, jchaffraix, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch
ap: review+, ap: commit-queue-
remove the comment none

Description Yong Li 2012-12-18 14:14:11 PST
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.
Comment 1 Yong Li 2012-12-19 07:28:25 PST
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
Comment 2 Yong Li 2013-02-11 10:06:06 PST
Created attachment 187607 [details]
the patch
Comment 3 Alexey Proskuryakov 2013-02-11 14:26:59 PST
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.
Comment 4 Yong Li 2013-02-11 14:38:36 PST
Created attachment 187679 [details]
remove the comment

Thanks Alexey
Comment 5 Allan Sandfeld Jensen 2013-02-11 14:59:19 PST
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.
Comment 6 Allan Sandfeld Jensen 2013-02-11 15:01:52 PST
(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.
Comment 7 Yong Li 2013-02-11 15:08:49 PST
(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.
Comment 8 Yong Li 2013-02-11 15:10:09 PST
(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?
Comment 9 Allan Sandfeld Jensen 2013-02-11 15:19:48 PST
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.
Comment 10 Allan Sandfeld Jensen 2013-02-11 15:27:09 PST
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.
Comment 11 Alexey Proskuryakov 2013-02-11 15:28:44 PST
> 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.
Comment 12 Allan Sandfeld Jensen 2013-02-11 15:45:08 PST
(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.
Comment 13 WebKit Review Bot 2013-02-11 16:03:18 PST
Comment on attachment 187679 [details]
remove the comment

Clearing flags on attachment: 187679

Committed r142538: <http://trac.webkit.org/changeset/142538>
Comment 14 WebKit Review Bot 2013-02-11 16:03:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Yong Li 2013-02-12 07:12:57 PST
Thanks Alan for double checking.