116886 2013-05-28 13:41:06 -0700 Rendering suppression extension tokens shouldn't be 0, should handle overflow 2013-05-28 14:54:51 -0700 1 1 1 Unclassified WebKit WebKit2 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 thorton thorton aestes commit-queue simon.fraser webkit-bug-importer oldest_to_newest 894491 0 thorton 2013-05-28 13:41:06 -0700 Apparently 0 is not a good value to try to insert into a HashSet. From http://trac.webkit.org/changeset/150388 894492 1 webkit-bug-importer 2013-05-28 13:42:13 -0700 <rdar://problem/14004474> 894504 2 203086 thorton 2013-05-28 14:14:22 -0700 Created attachment 203086 patch 894505 3 commit-queue 2013-05-28 14:15:52 -0700 Attachment 203086 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp']" exit_code: 1 Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4199: Missing space before ( in while( [whitespace/parens] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 894511 4 203086 darin 2013-05-28 14:20:18 -0700 Comment on attachment 203086 patch View in context: https://bugs.webkit.org/attachment.cgi?id=203086&action=review r=me assuming you deal with the 0xFFFFFFFF problem. >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4199 >> + while(!token || m_activeRenderingSuppressionTokens.contains(token)) > > Missing space before ( in while( [whitespace/parens] [5] First, let me say to the stylebot that I agree! Second, there are two values that a HashSet<unsigned> can’t handle. One is 0, and one is 0xFFFFFFFF. This code will call contains with 0xFFFFFFFF, which is a no-no. The way to deal with that is to use the isValidValue function instead of special casing zero. while (!token || !m_activeRenderingSuppressionTokens.isValidValue(token) || m_activeRenderingSuppressionTokens.contains(token)) There are two flaws with the code I pasted in here. One is that it’s probably more elegant to use a class name to call a static member function rather than using an instance to do so. The other is that it’s a little strange to have two checks for zero, one in this code and one in the isValidValue function; not sure the compiler will optimize one of them out. 894513 5 203086 darin 2013-05-28 14:22:07 -0700 Comment on attachment 203086 patch View in context: https://bugs.webkit.org/attachment.cgi?id=203086&action=review >>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4199 >>> + while(!token || m_activeRenderingSuppressionTokens.contains(token)) >> >> Missing space before ( in while( [whitespace/parens] [5] > > First, let me say to the stylebot that I agree! > > Second, there are two values that a HashSet<unsigned> can’t handle. One is 0, and one is 0xFFFFFFFF. This code will call contains with 0xFFFFFFFF, which is a no-no. The way to deal with that is to use the isValidValue function instead of special casing zero. > > while (!token || !m_activeRenderingSuppressionTokens.isValidValue(token) || m_activeRenderingSuppressionTokens.contains(token)) > > There are two flaws with the code I pasted in here. One is that it’s probably more elegant to use a class name to call a static member function rather than using an instance to do so. The other is that it’s a little strange to have two checks for zero, one in this code and one in the isValidValue function; not sure the compiler will optimize one of them out. You could just leave out the "!token" part if the only reason you were ruling out zero was to avoid the bad hash set value. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4200 > + token++; This ought to be: token = ++m_maximumRenderingSuppressionToken; Don’t you think? 894514 6 203086 aestes 2013-05-28 14:22:46 -0700 Comment on attachment 203086 patch View in context: https://bugs.webkit.org/attachment.cgi?id=203086&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4198 > + unsigned token = ++m_maximumRenderingSuppressionToken; You could make this 'unsigned token = m_maximumRenderingSuppressionToken + 1', since you'll update m_maximumRenderingSuppressionToken a few lines later. No need to update it twice. 894532 7 thorton 2013-05-28 14:47:43 -0700 (In reply to comment #6) > (From update of attachment 203086 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203086&action=review > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4198 > > + unsigned token = ++m_maximumRenderingSuppressionToken; > > You could make this 'unsigned token = m_maximumRenderingSuppressionToken + 1', since you'll update m_maximumRenderingSuppressionToken a few lines later. No need to update it twice. Agreed. (In reply to comment #5) > You could just leave out the "!token" part if the only reason you were ruling out zero was to avoid the bad hash set value. That is the only reason, so yeah, I'll leave it out. > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:4200 > > + token++; > > This ought to be: > > token = ++m_maximumRenderingSuppressionToken; > > Don’t you think? As Andy notes, I update m_maximumRenderingSuppressionToken a few lines below. 894534 8 thorton 2013-05-28 14:49:48 -0700 (In reply to comment #4) > (From update of attachment 203086 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=203086&action=review > > r=me assuming you deal with the 0xFFFFFFFF problem. Thanks! > First, let me say to the stylebot that I agree! Me too! > Second, there are two values that a HashSet<unsigned> can’t handle. One is 0, and one is 0xFFFFFFFF. This code will call contains with 0xFFFFFFFF, which is a no-no. The way to deal with that is to use the isValidValue function instead of special casing zero. I did not know that! 894541 9 thorton 2013-05-28 14:54:51 -0700 http://trac.webkit.org/changeset/150838 203086 2013-05-28 14:14:22 -0700 2013-05-28 14:22:46 -0700 patch incrender.diff text/plain 1576 thorton ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCBiY2UyNjJmLi5lZDM5MmNjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcg QEAKKzIwMTMtMDUtMjggIFRpbSBIb3J0b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CisK KyAgICAgICAgUmVuZGVyaW5nIHN1cHByZXNzaW9uIGV4dGVuc2lvbiB0b2tlbnMgc2hvdWxkbid0 IGJlIDAsIHNob3VsZCBoYW5kbGUgb3ZlcmZsb3cKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTExNjg4NgorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMTQw MDQ0NzQ+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg RG9uJ3QgdXNlIDAgYXMgYSB0b2tlbiwgYW5kIGVuc3VyZSB0aGF0IHdlJ3JlIG5vdCBhbHJlYWR5 IHVzaW5nIGEgdG9rZW4KKyAgICAgICAgYmVmb3JlIHJldHVybmluZyBpdC4KKworICAgICAgICAq IFdlYlByb2Nlc3MvV2ViUGFnZS9XZWJQYWdlLmNwcDoKKyAgICAgICAgKFdlYktpdDo6V2ViUGFn ZTo6ZXh0ZW5kSW5jcmVtZW50YWxSZW5kZXJpbmdTdXBwcmVzc2lvbik6CisKIDIwMTMtMDUtMjgg IExhdXJvIE5ldG8gPGxhdXJvLm5ldG9Ab3BlbmJvc3NhLm9yZz4KIAogICAgICAgICBbR1RLXSBD b25uZWN0aW9uIGlzc3VlcyBpbiByZXBlYXRlZCBXZWJQcm9jZXNzIGNyYXNoL3JlbG9hZHMuCmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvV2ViUGFnZS5jcHAg Yi9Tb3VyY2UvV2ViS2l0Mi9XZWJQcm9jZXNzL1dlYlBhZ2UvV2ViUGFnZS5jcHAKaW5kZXggNzZk NjRjMy4uNWNjMzA3OSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvV2ViUHJvY2Vzcy9XZWJQ YWdlL1dlYlBhZ2UuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1dlYlByb2Nlc3MvV2ViUGFnZS9X ZWJQYWdlLmNwcApAQCAtNDE5NSwxMSArNDE5NSwxNSBAQCB2b2lkIFdlYlBhZ2U6OnJlcG9ydFVz ZWRGZWF0dXJlcygpCiAKIHVuc2lnbmVkIFdlYlBhZ2U6OmV4dGVuZEluY3JlbWVudGFsUmVuZGVy aW5nU3VwcHJlc3Npb24oKQogewotICAgIHVuc2lnbmVkIHRva2VuID0gbV9tYXhpbXVtUmVuZGVy aW5nU3VwcHJlc3Npb25Ub2tlbisrOworICAgIHVuc2lnbmVkIHRva2VuID0gKyttX21heGltdW1S ZW5kZXJpbmdTdXBwcmVzc2lvblRva2VuOworICAgIHdoaWxlKCF0b2tlbiB8fCBtX2FjdGl2ZVJl bmRlcmluZ1N1cHByZXNzaW9uVG9rZW5zLmNvbnRhaW5zKHRva2VuKSkKKyAgICAgICAgdG9rZW4r KzsKIAogICAgIG1fYWN0aXZlUmVuZGVyaW5nU3VwcHJlc3Npb25Ub2tlbnMuYWRkKHRva2VuKTsK ICAgICBtX3BhZ2UtPm1haW5GcmFtZSgpLT52aWV3KCktPnNldFZpc3VhbFVwZGF0ZXNBbGxvd2Vk QnlDbGllbnQoZmFsc2UpOwogCisgICAgbV9tYXhpbXVtUmVuZGVyaW5nU3VwcHJlc3Npb25Ub2tl biA9IHRva2VuOworCiAgICAgcmV0dXJuIHRva2VuOwogfQogCg==