Bug 73351 – [Chromium] ASSERT fails in updateState ACTION_DRAW case

• Component: CSS
• Status: RESOLVED FIXED

[Armand]

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.

[James Robinson]

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?

[Nat Duca]

Armand, are you planning on putting up your patch, or is your expectation that someone else fixes this?

[Armand]

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.

[Armand]

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.

[Nat Duca]

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.

[Shane Stephens]

Armand, I've tentatively assigned this bug to you as you have a proposed fix for the issue.

[Armand]

Great.  I will submit my patch for this.  I'll try to get to it soon (next couple days).

[Armand]

Created attachment 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.

[James Robinson]

Comment on attachment 117716 [details]
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

[James Robinson]

(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.

[Armand]

Created attachment 117724 [details]
Patch

[James Robinson]

Comment on attachment 117724 [details]
Patch

Great! That's perfect.

[WebKit Review Bot]

Comment on attachment 117724 [details]
Patch

Attachment 117724 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10695457

New failing tests:
svg/custom/linking-uri-01-b.svg

[James Robinson]

Comment on attachment 117724 [details]
Patch

Flake, let's try again.

[WebKit Review Bot]

Comment on attachment 117724 [details]
Patch

Clearing flags on attachment: 117724

Committed r101911: <http://trac.webkit.org/changeset/101911>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.