Summary: | XMLHttpRequestProgressEventThrottle::resume() always schedules timer even when unnecessary | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yong Li <yong.li.webkit> | ||||||
Component: | Page Loading | Assignee: | 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
Yong Li
2012-12-18 14:14:11 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 Created attachment 187607 [details]
the patch
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. Created attachment 187679 [details]
remove the comment
Thanks Alexey
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.
(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. (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. (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? 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 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.
> 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.
(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 on attachment 187679 [details] remove the comment Clearing flags on attachment: 187679 Committed r142538: <http://trac.webkit.org/changeset/142538> All reviewed patches have been landed. Closing bug. Thanks Alan for double checking. |