73351 2011-11-29 12:43:42 -0800 [Chromium] ASSERT fails in updateState ACTION_DRAW case 2011-12-02 22:47:57 -0800 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Android Android RESOLVED FIXED P2 Normal --- 1 armand.navabi armand.navabi cc-bugs darin dglazkov jamesr nduca shanestephens webkit.review.bot oldest_to_newest 511018 0 armand.navabi 2011-11-29 12:43:42 -0800 The ASSERT in case ACTION_DRAW of CCSchedulerStateMachine::updateState(Action action) fails. A potential fix is to change the assert to ASSERT(m_needsCommit || !m_visible). The issue arises because updateState is being called right after m_needsCommit has been set to true and m_visible has been set to false. This will set m_commitState = COMMIT_STATE_WAITING_FOR_FIRST_DRAW in the ACTION_COMMIT case. Then on the draw action the assert is triggered and fails because it asserts only that m_needsCommit is true. Other solutions may be that m_needsCommit should not be set to true, m_visible may be incorrectly being set to false or m_needsForcedRedraw should not be set. The main question is what should the ASSERT statement be asserting in the ACTION_DRAW case of updateState. 511460 1 jamesr 2011-11-30 00:23:59 -0800 Are you talking about this assertion: case ACTION_DRAW: m_needsRedraw = false; m_needsForcedRedraw = false; if (m_insideVSync) m_lastFrameNumberWhereDrawWasCalled = m_currentFrameNumber; if (m_commitState == COMMIT_STATE_WAITING_FOR_FIRST_DRAW) { ASSERT(m_needsCommit); <----here m_commitState = COMMIT_STATE_IDLE; } return; ? If so I'm not sure how we are hitting this in the scenario you describe - if m_needsCommit is true then that assertion shouldn't fire. Or are you referring to a different assertion? 511756 2 nduca 2011-11-30 08:12:03 -0800 Armand, are you planning on putting up your patch, or is your expectation that someone else fixes this? 511927 3 armand.navabi 2011-11-30 11:17:43 -0800 Yes that is the failing ASSERT. I'm sorry, my initial report had a typo. The state we get into is one in which m_needsCommit is *false* and m_visible is *false*. In such a state, ACTION_COMMIT in updateState will set m_commitState = COMMIT_STATE_WAITING_FOR_FIRST_TO_DRAW (since !m_visible is true). Then, an ACTION_DRAW happens where m_commitState = COMMIT_STATE_WAITING_FOR_FIRST_TO_DRAW and m_needsCommit is false (and m_visible is false). Here the assertion fails. Nat: I was not planning on putting up my patch because I am not confident it is the correct solution. The patch I had was to change the assertion to ASSERT(m_needsCommit || !m_visible) in ACTION_DRAW. While this would solve this particular failure, I am not familiar enough with this code to know if this is correct. One question is, should ACTION_DRAW even be triggered when m_visible is false? If not, then the assertion in question is not the problem, but the problem is that ACTION_DRAW is being called even though m_visible is false. 511968 4 armand.navabi 2011-11-30 11:42:24 -0800 Yes that is the failing ASSERT. I'm sorry, my initial report had a typo. The state we get into is one in which m_needsCommit is *false* and m_visible is *false*. In such a state, ACTION_COMMIT in updateState will set m_commitState = COMMIT_STATE_WAITING_FOR_FIRST_TO_DRAW (since !m_visible is true). Then, an ACTION_DRAW happens where m_commitState = COMMIT_STATE_WAITING_FOR_FIRST_TO_DRAW and m_needsCommit is false (and m_visible is false). Here the assertion fails. Nat: I was not planning on putting up my patch because I am not confident it is the correct solution. The patch I had was to change the assertion to ASSERT(m_needsCommit || !m_visible) in ACTION_DRAW. While this would solve this particular failure, I am not familiar enough with this code to know if this is correct. One question is, should ACTION_DRAW even be triggered when m_visible is false? If not, then the assertion in question is not the problem, but the problem is that ACTION_DRAW is being called even though m_visible is false. 512198 5 nduca 2011-11-30 15:54:58 -0800 I liked your original patch, I think you were correct. I suggest putting it up here. I'll double check it, but in general, I think its good. 512349 6 shanestephens 2011-11-30 19:14:31 -0800 Armand, I've tentatively assigned this bug to you as you have a proposed fix for the issue. 513882 7 armand.navabi 2011-12-02 10:07:02 -0800 Great. I will submit my patch for this. I'll try to get to it soon (next couple days). 514295 8 117716 armand.navabi 2011-12-02 17:27:56 -0800 Created attachment 117716 Text file of patch (ChangeLog and diff) First patch I am submitting. This is for the following bug: https://bugs.webkit.org/show_bug.cgi?id=73351 Please let me know if I took the correct steps for future patches I submit. 514303 9 117716 jamesr 2011-12-02 17:42:24 -0800 Comment on attachment 117716 Text file of patch (ChangeLog and diff) View in context: https://bugs.webkit.org/attachment.cgi?id=117716&action=review > Source/WebCore/ChangeLog:10 > + Changed ASSERT in CCSchedulerStateMachine to include ( || !m_visible) as discussed in bug > + report. m_commitState is set to COMMIT_STATE_WAITING_FOR_FIRST_DRAW if m_needsCommit or > + !m_visible, so in ACTION_DRAW the assert should have both conditions. the intendentation here is off - see other entries in this file 514326 10 jamesr 2011-12-02 18:05:10 -0800 (In reply to comment #8) > Created an attachment (id=117716) [details] > Text file of patch (ChangeLog and diff) > > First patch I am submitting. This is for the following bug: > https://bugs.webkit.org/show_bug.cgi?id=73351 > > Please let me know if I took the correct steps for future patches I submit. I left one comment inline. Also, new patches should have the "review?" flag set. "webkit-patch upload" should take care of that automatically. 514333 11 117724 armand.navabi 2011-12-02 18:14:59 -0800 Created attachment 117724 Patch 514341 12 117724 jamesr 2011-12-02 18:19:48 -0800 Comment on attachment 117724 Patch Great! That's perfect. 514370 13 117724 webkit.review.bot 2011-12-02 19:09:37 -0800 Comment on attachment 117724 Patch Attachment 117724 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10695457 New failing tests: svg/custom/linking-uri-01-b.svg 514376 14 117724 jamesr 2011-12-02 19:19:08 -0800 Comment on attachment 117724 Patch Flake, let's try again. 514432 15 117724 webkit.review.bot 2011-12-02 22:47:51 -0800 Comment on attachment 117724 Patch Clearing flags on attachment: 117724 Committed r101911: <http://trac.webkit.org/changeset/101911> 514433 16 webkit.review.bot 2011-12-02 22:47:57 -0800 All reviewed patches have been landed. Closing bug. 117716 2011-12-02 17:27:56 -0800 2011-12-02 18:14:55 -0800 Text file of patch (ChangeLog and diff) bug73351.patch text/plain 1627 armand.navabi SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwMTg3MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDExLTEyLTAyICBBcm1hbmQg TmF2YWJpICA8YXJtYW5kLm5hdmFiaUBnbWFpbC5jb20+CisKKyAgICAgICAgQVNTRVJUIGZhaWxz IGluIHVwZGF0ZVN0YXRlIEFDVElPTl9EUkFXIGNhc2UKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTczMzUxCisJCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisJQ2hhbmdlZCBBU1NFUlQgaW4gQ0NTY2hlZHVsZXJTdGF0ZU1hY2hp bmUgdG8gaW5jbHVkZSAoIHx8ICFtX3Zpc2libGUpIGFzIGRpc2N1c3NlZCBpbiBidWcgCisJcmVw b3J0LiBtX2NvbW1pdFN0YXRlIGlzIHNldCB0byBDT01NSVRfU1RBVEVfV0FJVElOR19GT1JfRklS U1RfRFJBVyBpZiBtX25lZWRzQ29tbWl0IG9yCisJIW1fdmlzaWJsZSwgc28gaW4gQUNUSU9OX0RS QVcgdGhlIGFzc2VydCBzaG91bGQgaGF2ZSBib3RoIGNvbmRpdGlvbnMuCisKKyAgICAgICAgKiBw bGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9jYy9DQ1NjaGVkdWxlclN0YXRlTWFjaGluZS5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpDQ1NjaGVkdWxlclN0YXRlTWFjaGluZTo6dXBkYXRlU3RhdGUp OgorCiAyMDExLTEyLTAyICBBbmRyZWFzIEtsaW5nICA8a2xpbmdAd2Via2l0Lm9yZz4KIAogICAg ICAgICBTdHlsZWRFbGVtZW50OiBTaW1wbGlmeSBhZGRDU1NDb2xvcigpLgpJbmRleDogU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vY2MvQ0NTY2hlZHVsZXJTdGF0ZU1h Y2hpbmUuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNz L2Nocm9taXVtL2NjL0NDU2NoZWR1bGVyU3RhdGVNYWNoaW5lLmNwcAkocmV2aXNpb24gMTAxODQ0 KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vY2MvQ0NTY2hl ZHVsZXJTdGF0ZU1hY2hpbmUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMDYsNyArMTA2LDcgQEAg dm9pZCBDQ1NjaGVkdWxlclN0YXRlTWFjaGluZTo6dXBkYXRlU3RhdAogICAgICAgICBpZiAobV9p bnNpZGVWU3luYykKICAgICAgICAgICAgIG1fbGFzdEZyYW1lTnVtYmVyV2hlcmVEcmF3V2FzQ2Fs bGVkID0gbV9jdXJyZW50RnJhbWVOdW1iZXI7CiAgICAgICAgIGlmIChtX2NvbW1pdFN0YXRlID09 IENPTU1JVF9TVEFURV9XQUlUSU5HX0ZPUl9GSVJTVF9EUkFXKSB7Ci0gICAgICAgICAgICBBU1NF UlQobV9uZWVkc0NvbW1pdCk7CisgICAgICAgICAgICBBU1NFUlQobV9uZWVkc0NvbW1pdCB8fCAh bV92aXNpYmxlKTsKICAgICAgICAgICAgIG1fY29tbWl0U3RhdGUgPSBDT01NSVRfU1RBVEVfSURM RTsKICAgICAgICAgfQogICAgICAgICByZXR1cm47Cg== 117724 2011-12-02 18:14:59 -0800 2011-12-02 22:47:51 -0800 Patch bug-73351-20111202181458.patch text/plain 1647 armand.navabi SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwMTg3MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDExLTEyLTAyICBBcm1hbmQg TmF2YWJpICA8YXJtYW5kLm5hdmFiaUBnbWFpbC5jb20+CisKKyAgICAgICAgQVNTRVJUIGZhaWxz IGluIHVwZGF0ZVN0YXRlIEFDVElPTl9EUkFXIGNhc2UKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTczMzUxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgQ2hhbmdlZCBBU1NFUlQgaW4gQ0NTY2hlZHVsZXJTdGF0 ZU1hY2hpbmUgdG8gaW5jbHVkZSAoIHx8ICFtX3Zpc2libGUpIGFzIGRpc2N1c3NlZCBpbiBidWcg CisgICAgICAgIHJlcG9ydC4gbV9jb21taXRTdGF0ZSBpcyBzZXQgdG8gQ09NTUlUX1NUQVRFX1dB SVRJTkdfRk9SX0ZJUlNUX0RSQVcgaWYgbV9uZWVkc0NvbW1pdCBvcgorICAgICAgICAhbV92aXNp YmxlLCBzbyBpbiBBQ1RJT05fRFJBVyB0aGUgYXNzZXJ0IHNob3VsZCBoYXZlIGJvdGggY29uZGl0 aW9ucy4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0NDU2NoZWR1 bGVyU3RhdGVNYWNoaW5lLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNDU2NoZWR1bGVyU3RhdGVN YWNoaW5lOjp1cGRhdGVTdGF0ZSk6CisKIDIwMTEtMTItMDIgIEFuZHJlYXMgS2xpbmcgIDxrbGlu Z0B3ZWJraXQub3JnPgogCiAgICAgICAgIFN0eWxlZEVsZW1lbnQ6IFNpbXBsaWZ5IGFkZENTU0Nv bG9yKCkuCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9j Yy9DQ1NjaGVkdWxlclN0YXRlTWFjaGluZS5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vY2MvQ0NTY2hlZHVsZXJTdGF0ZU1hY2hpbmUu Y3BwCShyZXZpc2lvbiAxMDE4NDQpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGlj cy9jaHJvbWl1bS9jYy9DQ1NjaGVkdWxlclN0YXRlTWFjaGluZS5jcHAJKHdvcmtpbmcgY29weSkK QEAgLTEwNiw3ICsxMDYsNyBAQCB2b2lkIENDU2NoZWR1bGVyU3RhdGVNYWNoaW5lOjp1cGRhdGVT dGF0CiAgICAgICAgIGlmIChtX2luc2lkZVZTeW5jKQogICAgICAgICAgICAgbV9sYXN0RnJhbWVO dW1iZXJXaGVyZURyYXdXYXNDYWxsZWQgPSBtX2N1cnJlbnRGcmFtZU51bWJlcjsKICAgICAgICAg aWYgKG1fY29tbWl0U3RhdGUgPT0gQ09NTUlUX1NUQVRFX1dBSVRJTkdfRk9SX0ZJUlNUX0RSQVcp IHsKLSAgICAgICAgICAgIEFTU0VSVChtX25lZWRzQ29tbWl0KTsKKyAgICAgICAgICAgIEFTU0VS VChtX25lZWRzQ29tbWl0IHx8ICFtX3Zpc2libGUpOwogICAgICAgICAgICAgbV9jb21taXRTdGF0 ZSA9IENPTU1JVF9TVEFURV9JRExFOwogICAgICAgICB9CiAgICAgICAgIHJldHVybjsK