105348 2012-12-18 14:14:11 -0800 XMLHttpRequestProgressEventThrottle::resume() always schedules timer even when unnecessary 2013-02-12 07:12:57 -0800 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 yong.li.webkit webkit-unassigned allan.jensen ap jchaffraix webkit.review.bot oldest_to_newest 794034 0 yong.li.webkit 2012-12-18 14:14:11 -0800 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. 794652 1 yong.li.webkit 2012-12-19 07:28:25 -0800 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 829825 2 187607 yong.li.webkit 2013-02-11 10:06:06 -0800 Created attachment 187607 the patch 830132 3 187607 ap 2013-02-11 14:26:59 -0800 Comment on attachment 187607 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. 830148 4 187679 yong.li.webkit 2013-02-11 14:38:36 -0800 Created attachment 187679 remove the comment Thanks Alexey 830171 5 187679 allan.jensen 2013-02-11 14:59:19 -0800 Comment on attachment 187679 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. 830176 6 allan.jensen 2013-02-11 15:01:52 -0800 (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. 830184 7 yong.li.webkit 2013-02-11 15:08:49 -0800 (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. 830186 8 yong.li.webkit 2013-02-11 15:10:09 -0800 (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? 830200 9 allan.jensen 2013-02-11 15:19:48 -0800 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. 830209 10 187679 allan.jensen 2013-02-11 15:27:09 -0800 Comment on attachment 187679 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. 830215 11 ap 2013-02-11 15:28:44 -0800 > 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. 830242 12 allan.jensen 2013-02-11 15:45:08 -0800 (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. 830273 13 187679 webkit.review.bot 2013-02-11 16:03:18 -0800 Comment on attachment 187679 remove the comment Clearing flags on attachment: 187679 Committed r142538: <http://trac.webkit.org/changeset/142538> 830274 14 webkit.review.bot 2013-02-11 16:03:23 -0800 All reviewed patches have been landed. Closing bug. 830920 15 yong.li.webkit 2013-02-12 07:12:57 -0800 Thanks Alan for double checking. 187607 2013-02-11 10:06:06 -0800 2013-02-11 14:38:36 -0800 the patch 105348.diff text/plain 1750 yong.li.webkit ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAxZmVlM2Y1Li4wMWQwZGQ1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTgg QEAKKzIwMTMtMDItMTEgIFlvbmcgTGkgIDx5b2xpQHJpbS5jb20+CisKKyAgICAgICAgWE1MSHR0 cFJlcXVlc3RQcm9ncmVzc0V2ZW50VGhyb3R0bGU6OnJlc3VtZSgpIGFsd2F5cyBzY2hlZHVsZXMg dGltZXIgZXZlbiB3aGVuIHVubmVjZXNzYXJ5CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xMDUzNDgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBMZXQgcmVzdW1lKCkgY2xlYXIgdGhlIGRlZmVyIGZsYWcgYW5k IHJldHVybiBpZiB0aGVyZSBpcyBkZWZlcnJlZCBldmVudHMgdG8gZGlzcGF0Y2guCisKKyAgICAg ICAgTm8gbmV3IHRlc3RzIGFzIHRoaXMgc2hvdWxkIG5vdCBhZmZlY3QgZXhpc3RpbmcgY3Jvc3Mt cGxhdGZvcm0gYmVoYXZpb3IuIEl0IHNob3VsZCBiZQorICAgICAgICBPSyBhcyBsb25nIGFzIGl0 IGRvZXNuJ3QgYnJlYWsgYW55dGhpbmcuCisKKyAgICAgICAgKiB4bWwvWE1MSHR0cFJlcXVlc3RQ cm9ncmVzc0V2ZW50VGhyb3R0bGUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6WE1MSHR0cFJlcXVl c3RQcm9ncmVzc0V2ZW50VGhyb3R0bGU6OnJlc3VtZSk6CisKIDIwMTMtMDItMDcgIE1pa2hhaWwg UG96ZG55YWtvdiAgPG1pa2hhaWwucG96ZG55YWtvdkBpbnRlbC5jb20+CiAKICAgICAgICAgW1dL Ml1bRUZMXVtRVF1SRUdSRVNTSU9OKHIxNDIwNDUpOiBTY3JvbGxpbmcgaXMgYnJva2VuCmRpZmYg LS1naXQgYS9Tb3VyY2UvV2ViQ29yZS94bWwvWE1MSHR0cFJlcXVlc3RQcm9ncmVzc0V2ZW50VGhy b3R0bGUuY3BwIGIvU291cmNlL1dlYkNvcmUveG1sL1hNTEh0dHBSZXF1ZXN0UHJvZ3Jlc3NFdmVu dFRocm90dGxlLmNwcAppbmRleCAxM2UwY2FjLi4wNTVjMjJmIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS94bWwvWE1MSHR0cFJlcXVlc3RQcm9ncmVzc0V2ZW50VGhyb3R0bGUuY3BwCisrKyBi L1NvdXJjZS9XZWJDb3JlL3htbC9YTUxIdHRwUmVxdWVzdFByb2dyZXNzRXZlbnRUaHJvdHRsZS5j cHAKQEAgLTE5OCw2ICsxOTgsMTIgQEAgdm9pZCBYTUxIdHRwUmVxdWVzdFByb2dyZXNzRXZlbnRU aHJvdHRsZTo6cmVzdW1lKCkKICAgICBBU1NFUlQoIW1fbG9hZGVkKTsKICAgICBBU1NFUlQoIW1f dG90YWwpOwogCisgICAgaWYgKG1fZGVmZXJyZWRFdmVudHMuaXNFbXB0eSgpICYmICFtX2RlZmVy cmVkUHJvZ3Jlc3NFdmVudCkgeworICAgICAgICAvLyBUaGVyZSBpcyBubyBkZWZlcnJlZCBldmVu dC4gTGV0J3MgY2xlYXIgdGhlIGZsYWcgbm93LgorICAgICAgICBtX2RlZmVyRXZlbnRzID0gZmFs c2U7CisgICAgICAgIHJldHVybjsKKyAgICB9CisKICAgICAvLyBEbyBub3QgZGlzcGF0Y2ggZXZl bnRzIGlubGluZSBoZXJlLCBzaW5jZSBTY3JpcHRFeGVjdXRpb25Db250ZXh0IGlzIGl0ZXJhdGlu ZyBvdmVyCiAgICAgLy8gdGhlIGxpc3Qgb2YgYWN0aXZlIERPTSBvYmplY3RzIHRvIHJlc3VtZSB0 aGVtLCBhbmQgYW55IGFjdGl2YXRlZCBKUyBldmVudC1oYW5kbGVyCiAgICAgLy8gY291bGQgaW5z ZXJ0IG5ldyBhY3RpdmUgRE9NIG9iamVjdHMgdG8gdGhlIGxpc3QuCg== 187679 2013-02-11 14:38:36 -0800 2013-02-11 16:03:18 -0800 remove the comment 105348.diff text/plain 1689 yong.li.webkit ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAxZmVlM2Y1Li4wNDhiNDRkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTgg QEAKKzIwMTMtMDItMTEgIFlvbmcgTGkgIDx5b2xpQHJpbS5jb20+CisKKyAgICAgICAgWE1MSHR0 cFJlcXVlc3RQcm9ncmVzc0V2ZW50VGhyb3R0bGU6OnJlc3VtZSgpIGFsd2F5cyBzY2hlZHVsZXMg dGltZXIgZXZlbiB3aGVuIHVubmVjZXNzYXJ5CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xMDUzNDgKKworICAgICAgICBSZXZpZXdlZCBieSBBbGV4ZXkg UHJvc2t1cnlha292LgorCisgICAgICAgIExldCByZXN1bWUoKSBjbGVhciB0aGUgZGVmZXIgZmxh ZyBhbmQgcmV0dXJuIGlmIHRoZXJlIGlzIGRlZmVycmVkIGV2ZW50cyB0byBkaXNwYXRjaC4KKwor ICAgICAgICBObyBuZXcgdGVzdHMgYXMgdGhpcyBzaG91bGQgbm90IGFmZmVjdCBleGlzdGluZyBj cm9zcy1wbGF0Zm9ybSBiZWhhdmlvci4gSXQgc2hvdWxkIGJlCisgICAgICAgIE9LIGFzIGxvbmcg YXMgaXQgZG9lc24ndCBicmVhayBhbnl0aGluZy4KKworICAgICAgICAqIHhtbC9YTUxIdHRwUmVx dWVzdFByb2dyZXNzRXZlbnRUaHJvdHRsZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpYTUxIdHRw UmVxdWVzdFByb2dyZXNzRXZlbnRUaHJvdHRsZTo6cmVzdW1lKToKKwogMjAxMy0wMi0wNyAgTWlr aGFpbCBQb3pkbnlha292ICA8bWlraGFpbC5wb3pkbnlha292QGludGVsLmNvbT4KIAogICAgICAg ICBbV0syXVtFRkxdW1FUXVJFR1JFU1NJT04ocjE0MjA0NSk6IFNjcm9sbGluZyBpcyBicm9rZW4K ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3htbC9YTUxIdHRwUmVxdWVzdFByb2dyZXNzRXZl bnRUaHJvdHRsZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS94bWwvWE1MSHR0cFJlcXVlc3RQcm9ncmVz c0V2ZW50VGhyb3R0bGUuY3BwCmluZGV4IDEzZTBjYWMuLjgzNmY0ODEgMTAwNjQ0Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL3htbC9YTUxIdHRwUmVxdWVzdFByb2dyZXNzRXZlbnRUaHJvdHRsZS5jcHAK KysrIGIvU291cmNlL1dlYkNvcmUveG1sL1hNTEh0dHBSZXF1ZXN0UHJvZ3Jlc3NFdmVudFRocm90 dGxlLmNwcApAQCAtMTk4LDYgKzE5OCwxMSBAQCB2b2lkIFhNTEh0dHBSZXF1ZXN0UHJvZ3Jlc3NF dmVudFRocm90dGxlOjpyZXN1bWUoKQogICAgIEFTU0VSVCghbV9sb2FkZWQpOwogICAgIEFTU0VS VCghbV90b3RhbCk7CiAKKyAgICBpZiAobV9kZWZlcnJlZEV2ZW50cy5pc0VtcHR5KCkgJiYgIW1f ZGVmZXJyZWRQcm9ncmVzc0V2ZW50KSB7CisgICAgICAgIG1fZGVmZXJFdmVudHMgPSBmYWxzZTsK KyAgICAgICAgcmV0dXJuOworICAgIH0KKwogICAgIC8vIERvIG5vdCBkaXNwYXRjaCBldmVudHMg aW5saW5lIGhlcmUsIHNpbmNlIFNjcmlwdEV4ZWN1dGlvbkNvbnRleHQgaXMgaXRlcmF0aW5nIG92 ZXIKICAgICAvLyB0aGUgbGlzdCBvZiBhY3RpdmUgRE9NIG9iamVjdHMgdG8gcmVzdW1lIHRoZW0s IGFuZCBhbnkgYWN0aXZhdGVkIEpTIGV2ZW50LWhhbmRsZXIKICAgICAvLyBjb3VsZCBpbnNlcnQg bmV3IGFjdGl2ZSBET00gb2JqZWN0cyB0byB0aGUgbGlzdC4K