Bug 103959 – Coordinated Graphics: Refactor TiledBackingStore code in CoordinatedGraphicsLayer.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dongseong Hwang]

CoordinatedGraphicsLayer uses TiledBackingStore and the code is a bit hard to understand.
Several patches will clean up the complexity in this bug.

[Dongseong Hwang]

Created attachment 177422 [details]
Patch

[Dongseong Hwang]

I have run layout tests on WK2 debug and tests various pages. This patch endured all.

[Dongseong Hwang]

This bug physically, not logically, depends on Bug 103843.
If anyone reviews this bug, I can resolve the conflict.

[Dongseong Hwang]

Comment on attachment 177422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177422&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:212
> +    bool m_inCoordinateMode : 1;

actually, this flag is only for debugging. But I think it must exist. We can easily break this invariant.

[Kenneth Rohde Christiansen]

Comment on attachment 177422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177422&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:531
> +    if (!m_hasOwnTimers)
> +        return;

It is a bit hard to understand what this means, maybe we could name it differently

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:-680
> -bool CoordinatedGraphicsLayer::tiledBackingStoreUpdatesAllowed() const

I think this method was pretty much to the point

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:843
> +    setNeedsAdjustVisibleRect();

bad english, no better suggestions right now

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:720
> +                (*it)->setNeedsAdjustVisibleRect();

setNeedsVisibleRectAdjustment ?

[Dongseong Hwang]

Comment on attachment 177422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177422&action=review
Thank you for review!

>> Source/WebCore/platform/graphics/TiledBackingStore.cpp:531
>> +        return;
> 
> It is a bit hard to understand what this means, maybe we could name it differently

how about m_shouldUseTimersToCommit?

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:-680
>> -bool CoordinatedGraphicsLayer::tiledBackingStoreUpdatesAllowed() const
> 
> I think this method was pretty much to the point

I think this method is something like hacking.
I think it is more obvious for CoordinatedGraphicsLayer to call methods causing TBS updates when it is possible.
Moreover, we need to block CreateTile and RemoveTile messages as well as UpdateTile message. Currently, we send CreateTile and RemoveTile messages to UI Process at any time regardless of flushing, and it is ok now, but it is vulnerable. (In addition, it is blocker to implement actor model in Bug 103854)
This patch changes that CoordinatedGraphicsLayer calls methods of TBS causing CreateTile, UpdateTile and RemoveTile only during flushing. I think this explicit usage is easier to understand.

>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:843
>> +    setNeedsAdjustVisibleRect();
> 
> bad english, no better suggestions right now

setNeedsVisibleRectAdjustment is cool. thx for suggestion.

[Dongseong Hwang]

Created attachment 177449 [details]
Patch

[Kenneth Rohde Christiansen]

 
> > It is a bit hard to understand what this means, maybe we could name it differently
> 
> how about m_shouldUseTimersToCommit?

I think it is better to understand in what cases it needs that and when it doesn't. That makes the code easier to understand for someone who hasn't dealt with it before

[Dongseong Hwang]

Comment on attachment 177422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177422&action=review

>>> Source/WebCore/platform/graphics/TiledBackingStore.cpp:531
>>> +        return;
>> 
>> It is a bit hard to understand what this means, maybe we could name it differently
> 
> how about m_shouldUseTimersToCommit?

I changed m_allowInternalCommit in next patch. How about?

[Kenneth Rohde Christiansen]

> > how about m_shouldUseTimersToCommit?
> 
> I changed m_allowInternalCommit in next patch. How about?

I don't think that is more clear

Can you explain me when it is used instead? and when it is not used?

[Dongseong Hwang]

(In reply to comment #10)
> > > how about m_shouldUseTimersToCommit?
> > 
> > I changed m_allowInternalCommit in next patch. How about?
> 
> I don't think that is more clear
> 
> Can you explain me when it is used instead? and when it is not used?

Yes, my explanation was a bit vague. sorry.

CoordinatedGraphicsLayer does not want TiledBackingStore to use timers, because the timers will fire at any time and then TiledBackingStore causes calling createTile, updateTile and removeTile in CoordinatedGraphicsLayer.

For example, when CoordinatedGraphicsLayer calls TiledBackingStore::invalidate(), invalidate() start one shot of update timer. It causes calling updateTile regardless of CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly.
void TiledBackingStore::invalidate(const IntRect& contentsDirtyRect)
{
    ...
    startTileBufferUpdateTimer();
}

Another example, createTile can start one shot of create tile timer. It causes calling createTile and removeTile regardless of CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly.
void TiledBackingStore::createTiles()
{
    ....
    if (requiredTileCount)
        startBackingStoreUpdateTimer(m_tileCreationDelay);
}

so currently CoordinatedGraphicsLayer::createTile checks where m_coordinator is created. It makes us confused.
void CoordinatedGraphicsLayer::createTile(int tileID, const SurfaceUpdateInfo& updateInfo, const WebCore::IntRect& tileRect)
{
    if (m_coordinator)
        m_coordinator->createTile(id(), tileID, updateInfo, tileRect);
}

Even, createTile can be sent to UI Process even if the holder layer is not created yet.

m_allowInternalCommit plays a role as preventing TiledBackintStore from using Timer, because CoordinatedGraphicsLayer explicitly handles TiledBackingStore's all updates.
My bad English produces bad name continuously. Please suggest me :)

[Kenneth Rohde Christiansen]

ah so that explains it better.

I believe the commit code is due to the tiled backing store support in Qt / WebKit1 which is sync in nature.

This sounds like something that should be fixed in the TiledBackingStore instead, or potentially be removed from the TiledBackingStore and have the user do it instead.

Temporaly maybe we could add a 

class TiledBackingStore {
    // Used when class methods cannot be called asynchronously by client.
    // Changes are committed as soon as all the events in event queue have
    // been processed.
    setCommitOnIdleEventLoop(bool enable)
}

[Jocelyn Turcotte]

Comment on attachment 177449 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177449&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:546
> +    if (!m_allowInternalCommit)
> +        return;

createTiles() currently do the work in more than one step. It first creates all the tiles with the same manhatan distance to the viewport center, then start a timer and do the next distance, etc. This is done so that the user can see the rendered content for the viewport before we start rendering out-of-screen tiles.
By disabling the timer completely this will have the effect of bypassing the next timer and the cover rect will never be completely covered.
Ideally what we want is that tiles get created, that this schedules a layer flush, and that the next tile distance tiles are rendered in a new frame.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:766
> +        m_trajectoryVector = FloatPoint();

Maybe it would be better to set it explicitely on the TiledBackingStore and let coverWithTilesIfNeeded read the member instead of getting it through a parameter. Else please name it something like m_pendingTrajectoryVector to suggest that this is used temporarily.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:210
> -    bool m_inUpdateMode : 1;
> +    bool m_inCoordinateMode : 1;

Update is much clearer IMO

Please be careful when refactoring this kind of code as it is covered by almost no automated test. I personally don't see the value of this kind of change if it is going to cause more problems than it fixes.

[Jocelyn Turcotte]

(In reply to comment #13)
> Please be careful when refactoring this kind of code as it is covered by almost no automated test. I personally don't see the value of this kind of change if it is going to cause more problems than it fixes.

Maybe this sounds rude so I'd like to clarify my thoughts. We usually try to do refactoring only when doing so is going to be helpful for another change.

In other words, the behavior of the code is more important than how it looks (though the look is still important).

This has been frustrating since some of this kind of refactoring has been done lately, at the same time that we are trying to get our first WebKit2 release out with Qt 5.0 and I'm worried that some regressions might not have been noticed due to our lack of automated tests in this area.

[Dongseong Hwang]

(In reply to comment #12)
> ah so that explains it better.
> 
> I believe the commit code is due to the tiled backing store support in Qt / WebKit1 which is sync in nature.
> 
> This sounds like something that should be fixed in the TiledBackingStore instead, or potentially be removed from the TiledBackingStore and have the user do it instead.
> 
> Temporaly maybe we could add a 
> 
> class TiledBackingStore {
>     // Used when class methods cannot be called asynchronously by client.
>     // Changes are committed as soon as all the events in event queue have
>     // been processed.
>     setCommitOnIdleEventLoop(bool enable)
> }

Thank you. I'll use it :)


(In reply to comment #13)
> (From update of attachment 177449 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177449&action=review

Thank you for review. Your concern is very reasonable.

> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:546
> > +    if (!m_allowInternalCommit)
> > +        return;
> 
> createTiles() currently do the work in more than one step. It first creates all the tiles with the same manhatan distance to the viewport center, then start a timer and do the next distance, etc. This is done so that the user can see the rendered content for the viewport before we start rendering out-of-screen tiles.
> By disabling the timer completely this will have the effect of bypassing the next timer and the cover rect will never be completely covered.
> Ideally what we want is that tiles get created, that this schedules a layer flush, and that the next tile distance tiles are rendered in a new frame.

I did not catch yet. I'll change that TiledBackingStore::createTile can notify needToCreateMoreTile to client, and then CoordinatedGraphicsLayer schedules flushing one more time. This change can make the next tile distance tiles rendered in a new frame.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:766
> > +        m_trajectoryVector = FloatPoint();
> 
> Maybe it would be better to set it explicitely on the TiledBackingStore and let coverWithTilesIfNeeded read the member instead of getting it through a parameter. Else please name it something like m_pendingTrajectoryVector to suggest that this is used temporarily.

Good suggestion! I'll put coverWithTilesIfNeeded into TiledBackingStore.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:210
> > -    bool m_inUpdateMode : 1;
> > +    bool m_inCoordinateMode : 1;
> 
> Update is much clearer IMO
> 
(In reply to comment #14)
> (In reply to comment #13)
> > Please be careful when refactoring this kind of code as it is covered by almost no automated test. I personally don't see the value of this kind of change if it is going to cause more problems than it fixes.
> 
> Maybe this sounds rude so I'd like to clarify my thoughts. We usually try to do refactoring only when doing so is going to be helpful for another change.
> 
> In other words, the behavior of the code is more important than how it looks (though the look is still important).
> 
> This has been frustrating since some of this kind of refactoring has been done lately, at the same time that we are trying to get our first WebKit2 release out with Qt 5.0 and I'm worried that some regressions might not have been noticed due to our lack of automated tests in this area.

Very reasonable concern. I'll test more extensively.

I made this bug not for only readability. This is in preparation for refactoring TextureMapper to work in an actor model (Bug 103854).
You can see more detail in https://bugs.webkit.org/show_bug.cgi?id=103843#c12

I'll remove GraphicsLayerTextureMapper in WK2, specifically in Coordinated Graphics. Before that, I need LayerTreeCoordinator to pass message to UI Process by the RIGHT order.
This patch is necessary for that. I'll make the next patch carefully.

When will you close revision for release? I plan huge refactoring. I wish make it after your release.

[Zeno Albisser]

(In reply to comment #15)
> I made this bug not for only readability. This is in preparation for refactoring TextureMapper to work in an actor model (Bug 103854).

I am sorry to ask this naive question, but I still do not understand the real benefit of all that.
As Jocelyn just mentioned before, code readability is important but not the main goal here. Not breaking existing code and keep things running without causing a lot of disruptions are much more important in my opinion.
Unless of course, this is a topic that has been discussed before on the mailing list and a consensus for such deep going changes has been reached. - But maybe i missed that.

> You can see more detail in https://bugs.webkit.org/show_bug.cgi?id=103843#c12

As far as i see, you are listing two benefits. Readability (see above) and avoid potential bugs.
I personally don't think that avoidance of potential bugs outweighs the risk of introducing a lot of new bugs for several ports by rewriting/refactoring an integral part of our infrastructure.

> 
> I'll remove GraphicsLayerTextureMapper in WK2, specifically in Coordinated Graphics. Before that, I need LayerTreeCoordinator to pass message to UI Process by the RIGHT order.
> This patch is necessary for that. I'll make the next patch carefully.
> 
> When will you close revision for release? I plan huge refactoring. I wish make it after your release.

Could you please let us know about such plans in advance and may be discuss that upfront on the mailing list? I am sure that you are planning on making valuable contributions that are very welcome.
But to be able to understand why you are making such changes it would be very helpful to understand the exact need for a change as well as the significance etc.

[Jocelyn Turcotte]

(In reply to comment #15)
> > createTiles() currently do the work in more than one step. It first creates all the tiles with the same manhatan distance to the viewport center, then start a timer and do the next distance, etc. This is done so that the user can see the rendered content for the viewport before we start rendering out-of-screen tiles.
> > By disabling the timer completely this will have the effect of bypassing the next timer and the cover rect will never be completely covered.
> > Ideally what we want is that tiles get created, that this schedules a layer flush, and that the next tile distance tiles are rendered in a new frame.
> 
> I did not catch yet. I'll change that TiledBackingStore::createTile can notify needToCreateMoreTile to client, and then CoordinatedGraphicsLayer schedules flushing one more time. This change can make the next tile distance tiles rendered in a new frame.

That sounds like a reasonable aproach.

> I made this bug not for only readability. This is in preparation for refactoring TextureMapper to work in an actor model (Bug 103854).
> You can see more detail in https://bugs.webkit.org/show_bug.cgi?id=103843#c12

> > 
> > I'll remove GraphicsLayerTextureMapper in WK2, specifically in Coordinated Graphics. Before that, I need LayerTreeCoordinator to pass message to UI Process by the RIGHT order.
> > This patch is necessary for that. I'll make the next patch carefully.
> > 
> > When will you close revision for release? I plan huge refactoring. I wish make it after your release.
> 
> Could you please let us know about such plans in advance and may be discuss that upfront on the mailing list? I am sure that you are planning on making valuable contributions that are very welcome.
> But to be able to understand why you are making such changes it would be very helpful to understand the exact need for a change as well as the significance etc.

I agree with it, this sounds like a nice project but I didn't hear about it yet, maybe Noam did.
Do you have a prototype already of it working on a branch somewhere? What performance gain were you or think you will be able to achieve? How can I know that this is a good way forward and that it is worth investing the stability of the code we share for your researchs? What is the benefit for the Qt and EFL ports?
I'm asking because there is nothing justifying a "huge refactoring" right now, at least not with the information that I have.

The branch has been created this weekend. We are going to be cherry-picking patches intensively for the next few weeks so if you can hold it until the end of December it would be great.

[Noam Rosenthal]

> I agree with it, this sounds like a nice project but I didn't hear about it yet, maybe Noam did.
> Do you have a prototype already of it working on a branch somewhere? What performance gain were you or think you will be able to achieve? How can I know that this is a good way forward and that it is worth investing the stability of the code we share for your researchs? What is the benefit for the Qt and EFL ports?
> I'm asking because there is nothing justifying a "huge refactoring" right now, at least not with the information that I have.

From my understanding there are three benefits for this project:
1. allowing threaded composition in WebKit1
2. sharing more code between the WebKit1 implementation and the WebKit2 implementation
3. Enabling GTK to use coordinated graphics (though, only in a threaded way).

Those to me look like reasonable goals, though not what I'm currently focusing on outside of reviews, plus I saw the research done on company100's branch and it looked pretty sane, also they have sent an email to webkit-dev and there was not much response/opposition.
I suggested that refactoring TextureMapper+CoordinatedGraphics to act as one unit would make more sense than creating a huge ThreadedCoordinatedGraphics parallel universe.

However of course this is not my personal decision to make and so far I felt like the patches I reviewed took the code forward in other ways regardless of the suggested refactor, which I hope people give more comments about.

[Jocelyn Turcotte]

(In reply to comment #18)
Thanks, this is the information I wanted, I agree that this sounds like a valuable effort.
I'm not paying so much attention to webkit-dev :)

Sorry for adding noise to this bug report, this is only indirectly related.

[Dongseong Hwang]

Thanks for your valuable feedback!
Thank noam for explanation. And sorry Jocelyn and Zeno for not enough sharing the plan.

I'll share updated plan in webkit-dev again.

Bug 100341 is Thread Coordinated Graphics meta bug.
Bug 102994 is Thread Coordinated Graphics on Qt WK1 meta bug.

This bug is under Bug 102994.

I have asked noam about this project in #qtwekit IRC. I should share before running.

Before implementing threaded coordinated graphics, I need two kinds of refactoring.

1. Separate LayerTreeCoordinator into LayerTreeHost role and other roles (i.e. GraphicsLayerClient, CoordinatedGraphicsLayerClient, CoordinatedImageBacking::Coordinator, UpdateAtlasClient, GraphicsLayerFactory, WebCustomFilterProgramProxyClient).
Could you suggest good name? LayerTreeCoordinator and CompositingCoordinator? TextureMapperCoordinator? CoordinatedGraphicsCoordinator? 

2. Refactor TextureMapperLayer to work in an actor model. It will remove GraphicsLayerTextureMapper in coordinated graphics.
After refactoring, TextureMapperLayer does not know GraphicsLayerTextureMapper. GraphicsLayerTextureMapper and LayerTreeRender will use TextureMapperLayer in the same way: message passing (actor model)
Also coordinated graphics has only one GraphicsLayer implementaion: CoordinatedGraphicsLayer.
In addition, LayerTreeRenderer does not need control logic to handle layer tree. LayerTreeRenderer will redirect messages from CoordinatedGraphicsLayer to TextureMapperLayer.
For example, LayerTreeRenderer needs to traverse layer tree one more time although LayerTreeCoordinator traverses layer tree earlier in that LayerTreeRenderer::flushLayerChanges() calls GraphicsLayerTextureMapper::flushCompositingState().
After refactoring, above redundant tree traversing is not needed.

This bug prepares for #2.
Before refactor actor model, I need LayerTreeCoordinator to send messages to UI Process by RIGHT order. And I expect some bugs can be fixed, which occur only in WK2 in spite of well in WK1.

[Dongseong Hwang]

Hi, folks.
https://bugs.webkit.org/show_bug.cgi?id=102994#c1
In above comment, I explained how we will refactor coordinated graphics.
Any comments/concerns are appreciated!

[Dongseong Hwang]

Created attachment 177915 [details]
Patch

[Dongseong Hwang]

Comment on attachment 177449 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177449&action=review

>>> Source/WebCore/platform/graphics/TiledBackingStore.cpp:546
>>> +        return;
>> 
>> createTiles() currently do the work in more than one step. It first creates all the tiles with the same manhatan distance to the viewport center, then start a timer and do the next distance, etc. This is done so that the user can see the rendered content for the viewport before we start rendering out-of-screen tiles.
>> By disabling the timer completely this will have the effect of bypassing the next timer and the cover rect will never be completely covered.
>> Ideally what we want is that tiles get created, that this schedules a layer flush, and that the next tile distance tiles are rendered in a new frame.
> 
> I did not catch yet. I'll change that TiledBackingStore::createTile can notify needToCreateMoreTile to client, and then CoordinatedGraphicsLayer schedules flushing one more time. This change can make the next tile distance tiles rendered in a new frame.

Done. I added TiledBackingStoreClient::tiledBackingStoreNeedToCreateNextTiles()

>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:766
>>> +        m_trajectoryVector = FloatPoint();
>> 
>> Maybe it would be better to set it explicitely on the TiledBackingStore and let coverWithTilesIfNeeded read the member instead of getting it through a parameter. Else please name it something like m_pendingTrajectoryVector to suggest that this is used temporarily.
> 
> Good suggestion! I'll put coverWithTilesIfNeeded into TiledBackingStore.

Done. I made TiledBackingStore::setTrajectoryVector.

>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:210
>>> +    bool m_inCoordinateMode : 1;
>> 
>> Update is much clearer IMO
>> 
>> Please be careful when refactoring this kind of code as it is covered by almost no automated test. I personally don't see the value of this kind of change if it is going to cause more problems than it fixes.
> 
> (In reply to comment #14)

I rollbacked

[Dongseong Hwang]

Created attachment 177917 [details]
Patch

[Dongseong Hwang]

(In reply to comment #24)
> Created an attachment (id=177917) [details]
> Patch

Rebase to upstream

[Kenneth Rohde Christiansen]

Comment on attachment 177917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177917&action=review

> Source/WebCore/page/Frame.cpp:824
> +    // Changes are committed by TiledBackingStore
> +    m_tiledBackingStore->setCommitOnIdleEventLoop(true);

im not sure that needs a comment, anyway, dot at end

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:75
> +    bool geometryUpdated = m_trajectoryVector != m_pendingTrajectoryVector || m_visibleRect != visibleRect || m_rect != rect;

didChange ?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:333
> +void TiledBackingStore::needToCreateNextTiles()

create next tiles sounds weird to me

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:788
> +        bool forceCreateTiles = m_shouldCreateNextTiles;

forceTileCreation

[Dongseong Hwang]

Created attachment 177983 [details]
Patch

[Dongseong Hwang]

(In reply to comment #26)
> (From update of attachment 177917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177917&action=review

Thank you for review!!

> > Source/WebCore/page/Frame.cpp:824
> > +    // Changes are committed by TiledBackingStore
> > +    m_tiledBackingStore->setCommitOnIdleEventLoop(true);
> 
> im not sure that needs a comment, anyway, dot at end

I think so. I removed redundant comment :)

> 
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:75
> > +    bool geometryUpdated = m_trajectoryVector != m_pendingTrajectoryVector || m_visibleRect != visibleRect || m_rect != rect;
> 
> didChange ?

Done.

> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:333
> > +void TiledBackingStore::needToCreateNextTiles()
> 
> create next tiles sounds weird to me

Changed needToCreateTiles. Also Change all code from [XXX]CreateNextTiles to [XXX]CreateTiles.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:788
> > +        bool forceCreateTiles = m_shouldCreateNextTiles;
> 
> forceTileCreation

Thx!

[Jocelyn Turcotte]

Comment on attachment 177983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177983&action=review

> Source/WebKit2/ChangeLog:23
> +        In addition, the purpose of CoordinatedGraphicsLayer::m_inUpdateMode is
> +        changed. Currently, we set m_inUpdateMode to true when updating a
> +        backing store. After this patch, we set m_inUpdateMode to true when
> +        sending messages via LayerTreeCoordinator to UI Process.

I guess that explains why you wanted to rename it.
I don't see where you read that variable, I only see it in assert. What is it used for?
If you are only going to use it in a subsequent patch, then please just remove m_inUpdateMode in this patch and add the new variable once you need it (with a new name if that fits better the new purpose).

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:70
> +void TiledBackingStore::createTilesIfNeeded(bool forceTileCreation)

I think it would be better to keep a flag under "if (requiredTileCount)" in TiledBackingStore::createTiles to know if we need a "next run" and improve encapsulation. The calling code shouldn't have to worry about this.

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:534
> +    if (!m_commitOnIdleEventLoop)
> +        return;

You could also put an ASSERT(m_commitOnIdleEventLoop); in startTileBufferUpdateTimer and tileBufferUpdateTimerFired.

> Source/WebCore/platform/graphics/TiledBackingStore.h:52
> +    // Used when class methods cannot be called asynchronously by client.
> +    // Changes are committed as soon as all the events in event queue have
> +    // been processed.
> +    void setCommitOnIdleEventLoop(bool enable) { m_commitOnIdleEventLoop = enable; }

I'm still not convinced about that name, what does "commit" mean?
How about something like triggerTileUpdatesAutomatically, or set it reverse with delegateTileUpdatesToClient?
Anyway I guess it doesn't matter so much, so setCommitOnIdleEventLoop might be fine too.

[Jocelyn Turcotte]

(In reply to comment #29)
> You could also put an ASSERT(m_commitOnIdleEventLoop); in startTileBufferUpdateTimer and tileBufferUpdateTimerFired.

Sorry, I meant backingStoreUpdateTimerFired and tileBufferUpdateTimerFired, not startTileBufferUpdateTimer.

[Dongseong Hwang]

(In reply to comment #29)
> (From update of attachment 177983 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177983&action=review

Thanks for review :)

> > Source/WebKit2/ChangeLog:23
> > +        In addition, the purpose of CoordinatedGraphicsLayer::m_inUpdateMode is
> > +        changed. Currently, we set m_inUpdateMode to true when updating a
> > +        backing store. After this patch, we set m_inUpdateMode to true when
> > +        sending messages via LayerTreeCoordinator to UI Process.
> 
> I guess that explains why you wanted to rename it.
> I don't see where you read that variable, I only see it in assert. What is it used for?
Yes, only debug purpose now. Is it better to put #ifndef NDEBUG ?
> If you are only going to use it in a subsequent patch, then please just remove m_inUpdateMode in this patch and add the new variable once you need it (with a new name if that fits better the new purpose).
I have no plan to post subsequent patch.
But ASSETION check is very important, because of telling other developers what is invariant. I can not remove ASSETIONs :)

> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:70
> > +void TiledBackingStore::createTilesIfNeeded(bool forceTileCreation)
> 
> I think it would be better to keep a flag under "if (requiredTileCount)" in TiledBackingStore::createTiles to know if we need a "next run" and improve encapsulation. The calling code shouldn't have to worry about this.

Good suggestion! I'll do that!

> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:534
> > +    if (!m_commitOnIdleEventLoop)
> > +        return;
> 
> You could also put an ASSERT(m_commitOnIdleEventLoop); in startTileBufferUpdateTimer and tileBufferUpdateTimerFired.

Good suggestion too!

> > Source/WebCore/platform/graphics/TiledBackingStore.h:52
> > +    // Used when class methods cannot be called asynchronously by client.
> > +    // Changes are committed as soon as all the events in event queue have
> > +    // been processed.
> > +    void setCommitOnIdleEventLoop(bool enable) { m_commitOnIdleEventLoop = enable; }
> 
> I'm still not convinced about that name, what does "commit" mean?
> How about something like triggerTileUpdatesAutomatically, or set it reverse with delegateTileUpdatesToClient?
> Anyway I guess it doesn't matter so much, so setCommitOnIdleEventLoop might be fine too.

kenneth suggested setCommitOnIdleEventLoop because my naming is bad: shouldUseTimersToCommit
I choose now among three: setCommitOnIdleEventLoop, triggerTileUpdatesAutomatically, delegateTileUpdatesToClient.
triggerTileUpdatesAutomatically is selected :)

[Dongseong Hwang]

Created attachment 178153 [details]
Patch

[Kenneth Rohde Christiansen]

> > I'm still not convinced about that name, what does "commit" mean?
> > How about something like triggerTileUpdatesAutomatically, or set it reverse with delegateTileUpdatesToClient?
> > Anyway I guess it doesn't matter so much, so setCommitOnIdleEventLoop might be fine too.
> 
> kenneth suggested setCommitOnIdleEventLoop because my naming is bad: shouldUseTimersToCommit
> I choose now among three: setCommitOnIdleEventLoop, triggerTileUpdatesAutomatically, delegateTileUpdatesToClient.
> triggerTileUpdatesAutomatically is selected :)

But then it is not so obvious what it does. It is not just triggering automatically. The idea is that it doesn't "commit" the changes immediately but first when the event loop is idle. setCommitTileUpdatesOnIdleEventLoop() ?

[Dongseong Hwang]

Created attachment 178337 [details]
Patch

[Dongseong Hwang]

(In reply to comment #33)
> But then it is not so obvious what it does. It is not just triggering automatically. The idea is that it doesn't "commit" the changes immediately but first when the event loop is idle. setCommitTileUpdatesOnIdleEventLoop() ?

setCommitTileUpdatesOnIdleEventLoop is good! I reposted.

[Dongseong Hwang]

Created attachment 178338 [details]
Patch

[Dongseong Hwang]

Created attachment 178340 [details]
Patch

[Dongseong Hwang]

I'm looking forward a folk reviewing this bug.

[Kenneth Rohde Christiansen]

Comment on attachment 178340 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178340&action=review

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:50
> +    , m_needToCreateNextTiles(false)

I dont understand this variable

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:191
> -    coverWithTilesIfNeeded();
> +    createTilesIfNeeded();

why this rename, it doesnt delete tiles anymore?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:330
> +    m_needToCreateNextTiles = requiredTileCount;

m_pendingTileCreation ?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:333
> +    if (m_needToCreateNextTiles)
> +        needToCreateTiles();
> +}

it is only called here. Why have the method?

> Source/WebCore/platform/graphics/TiledBackingStore.cpp:338
> +        m_client->tiledBackingStoreNeedToCreateTiles();

pendingTileCreation?

[Dongseong Hwang]

Created attachment 178749 [details]
Patch

[Dongseong Hwang]

(In reply to comment #39)
> (From update of attachment 178340 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178340&action=review

Thank you for review!

> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:50
> > +    , m_needToCreateNextTiles(false)
> 
> I dont understand this variable

It is m_pendingTileCreation now!

> 
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:191
> > -    coverWithTilesIfNeeded();
> > +    createTilesIfNeeded();
> 
> why this rename, it doesnt delete tiles anymore?

rollbacked the name because ti still delete tiles.

> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:330
> > +    m_needToCreateNextTiles = requiredTileCount;
> 
> m_pendingTileCreation ?
> 
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:333
> > +    if (m_needToCreateNextTiles)
> > +        needToCreateTiles();
> > +}
> 
> it is only called here. Why have the method?
> 
> > Source/WebCore/platform/graphics/TiledBackingStore.cpp:338
> > +        m_client->tiledBackingStoreNeedToCreateTiles();
> 
> pendingTileCreation?

Renamed tiledBackingStoreHasPendingTileCreation()

[Kenneth Rohde Christiansen]

Comment on attachment 178749 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178749&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:664
> +        m_shouldAdjustContentsScale = true;

is this really a should? or a m_pendingContentsScaleChange ?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:774
> +    ASSERT(m_inUpdateMode);

do we want to commit this inUpdateMode debugging? If so we need a better name

[Dongseong Hwang]

(In reply to comment #42)
> (From update of attachment 178749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178749&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:664
> > +        m_shouldAdjustContentsScale = true;
> 
> is this really a should? or a m_pendingContentsScaleChange ?

m_pendingContentsScaleChange is right.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:774
> > +    ASSERT(m_inUpdateMode);
> 
> do we want to commit this inUpdateMode debugging? If so we need a better name

only for debugging. How about m_inUpdateModeForDebugging?

[Dongseong Hwang]

Created attachment 178755 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 178755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178755&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:798
> +    if (m_shouldAdjustVisibleRect) {
> +        m_shouldAdjustVisibleRect = false;

Then this should also be m_pendingVisibleRectChange

[Dongseong Hwang]

Comment on attachment 178755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178755&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:129
> +    , m_shouldAdjustVisibleRect(false)

If we change m_pendingVisibleRectChange, it is confused with existing m_shouldUpdateVisibleRect member.
How about m_pendingVisibleRectChangeOfBackingStore?
If so, we need to change setNeedsVisibleRectAdjustment() also. maybe setNeedsVisibleRectChangeOfBackingStore()?

[Dongseong Hwang]

(In reply to comment #46)
> (From update of attachment 178755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178755&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:129
> > +    , m_shouldAdjustVisibleRect(false)
> 
> If we change m_pendingVisibleRectChange, it is confused with existing m_shouldUpdateVisibleRect member.
> How about m_pendingVisibleRectChangeOfBackingStore?
> If so, we need to change setNeedsVisibleRectAdjustment() also. maybe setNeedsVisibleRectChangeOfBackingStore()?

m_pendingContentsScaleChange is not for only backingstore, so we don't need to change m_pendingContentsScaleChangeOfBackingStore.

[Kenneth Rohde Christiansen]

Comment on attachment 178755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178755&action=review

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:118
>      , m_shouldUpdateVisibleRect(true)

Maybe this is more like m_pendingVisibleRectChangeForChildren ?

>>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:129
>>> +    , m_shouldAdjustVisibleRect(false)
>> 
>> If we change m_pendingVisibleRectChange, it is confused with existing m_shouldUpdateVisibleRect member.
>> How about m_pendingVisibleRectChangeOfBackingStore?
>> If so, we need to change setNeedsVisibleRectAdjustment() also. maybe setNeedsVisibleRectChangeOfBackingStore()?
> 
> m_pendingContentsScaleChange is not for only backingstore, so we don't need to change m_pendingContentsScaleChangeOfBackingStore.

Both are quite confusing today.

First of all, are the contents scale and visible rect ever used independently? if not we could use m_pendingBackingStoreChange

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:934
>      // The combined transform will be used in tiledBackingStoreVisibleRect.
> -    adjustVisibleRect();
> -    adjustContentsScale();
> +    setNeedsVisibleRectAdjustment();
>  }

This code here is checked for shouldUpdateVisibleContentsRect, maybe we dont need that anymore

[Dongseong Hwang]

(In reply to comment #48)
> (From update of attachment 178755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178755&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:118
> >      , m_shouldUpdateVisibleRect(true)
> 
> Maybe this is more like m_pendingVisibleRectChangeForChildren ?

m_shouldUpdateVisibleRect exists not for only children. I think m_shouldUpdateVisibleRect should be remained to match 'should' prefix of other members:
    bool m_shouldSyncLayerState: 1;
    bool m_shouldSyncChildren: 1;
    bool m_shouldSyncFilters: 1;
    bool m_shouldSyncImageBacking: 1;
    bool m_shouldSyncAnimations: 1;

> 
> >>> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:129
> >>> +    , m_shouldAdjustVisibleRect(false)
> >> 
> >> If we change m_pendingVisibleRectChange, it is confused with existing m_shouldUpdateVisibleRect member.
> >> How about m_pendingVisibleRectChangeOfBackingStore?
> >> If so, we need to change setNeedsVisibleRectAdjustment() also. maybe setNeedsVisibleRectChangeOfBackingStore()?
> > 
> > m_pendingContentsScaleChange is not for only backingstore, so we don't need to change m_pendingContentsScaleChangeOfBackingStore.
> 
> Both are quite confusing today.
> 
> First of all, are the contents scale and visible rect ever used independently? if not we could use m_pendingBackingStoreChange

We must use independently. m_pendingContentsScaleChange affects m_previousBackingStore.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:934
> >      // The combined transform will be used in tiledBackingStoreVisibleRect.
> > -    adjustVisibleRect();
> > -    adjustContentsScale();
> > +    setNeedsVisibleRectAdjustment();
> >  }
> 
> This code here is checked for shouldUpdateVisibleContentsRect, maybe we dont need that anymore
We still need. setNeedsVisibleRectAdjustment() set m_shouldAdjustVisibleRect, not m_shouldUpdateVisibleRect.


In conclusion, I'll change from m_pendingContentsScaleChange and m_shouldAdjustVisibleRect to m_pendingContentsScaleAdjustment and m_pendingVisibleRectAdjustment respectively.
We have used adjustment term to manipulate backing store and both rhymes with 'pending' prefix.

[Dongseong Hwang]

Created attachment 178939 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 178939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=178939&action=review

r=me with the change suggested

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:814
> +    TemporaryChange<bool> updateModeProtector(m_isFlushingLayerChanges, true);

just call this protector please

[Dongseong Hwang]

Created attachment 178997 [details]
Patch

[Dongseong Hwang]

(In reply to comment #51)
> (From update of attachment 178939 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178939&action=review
> 
> r=me with the change suggested
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:814
> > +    TemporaryChange<bool> updateModeProtector(m_isFlushingLayerChanges, true);
> 
> just call this protector please

Oops, I missed. Done :)

[WebKit Review Bot]

Comment on attachment 178997 [details]
Patch

Clearing flags on attachment: 178997

Committed r137438: <http://trac.webkit.org/changeset/137438>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

This patch is asserting on EFL Debug bots. 400+ new failures.

crash log for WebProcess (pid <unknown>):
STDOUT: <empty>
STDERR: ASSERTION FAILED: m_isFlushingLayerChanges
STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp(774) : virtual void WebCore::CoordinatedGraphicsLayer::removeTile(uint32_t)
STDERR: 1   0x7ff1f68d87d8 WebCore::CoordinatedGraphicsLayer::removeTile(unsigned int)
STDERR: 2   0x7ff1f68dd00b WebKit::CoordinatedTile::~CoordinatedTile()
STDERR: 3   0x7ff1f68dd05a WebKit::CoordinatedTile::~CoordinatedTile()
STDERR: 4   0x7ff1f68ddb6e WTF::RefCounted<WebCore::Tile>::deref()
STDERR: 5   0x7ff1f2823761 void WTF::derefIfNotNull<WebCore::Tile>(WebCore::Tile*)
STDERR: 6   0x7ff1f2822e6b WTF::RefPtr<WebCore::Tile>::~RefPtr()
STDERR: 7   0x7ff1f28236c6 WTF::KeyValuePair<WebCore::IntPoint, WTF::RefPtr<WebCore::Tile> >::~KeyValuePair()
STDERR: 8   0x7ff1f282368b WTF::HashTable<WebCore::IntPoint, WTF::KeyValuePair<WebCore::IntPoint, WTF::RefPtr<WebCore::Tile> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::IntPoint, WTF::RefPtr<WebCore::Tile> > >, WTF::IntPointHash, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::IntPoint>, WTF::HashTraits<WTF::RefPtr<WebCore::Tile> > >, WTF::HashTraits<WebCore::IntPoint> >::deallocateTable(WTF::KeyValuePair<WebCore::IntPoint, WTF::RefPtr<WebCore::Tile> >*, int)
STDERR: 9   0x7ff1f2822d72 WTF::HashTable<WebCore::IntPoint, WTF::KeyValuePair<WebCore::IntPoint, WTF::RefPtr<WebCore::Tile> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::IntPoint, WTF::RefPtr<WebCore::Tile> > >, WTF::IntPointHash, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::IntPoint>, WTF::HashTraits<WTF::RefPtr<WebCore::Tile> > >, WTF::HashTraits<WebCore::IntPoint> >::~HashTable()
STDERR: 10  0x7ff1f2822b74 WTF::HashMap<WebCore::IntPoint, WTF::RefPtr<WebCore::Tile>, WTF::IntPointHash, WTF::HashTraits<WebCore::IntPoint>, WTF::HashTraits<WTF::RefPtr<WebCore::Tile> > >::~HashMap()
STDERR: 11  0x7ff1f282025c WebCore::TiledBackingStore::~TiledBackingStore()
STDERR: 12  0x7ff1f68db36e void WTF::deleteOwnedPtr<WebCore::TiledBackingStore>(WebCore::TiledBackingStore*)
STDERR: 13  0x7ff1f68dac69 WTF::OwnPtr<WebCore::TiledBackingStore>::~OwnPtr()
STDERR: 14  0x7ff1f68d614e WebCore::CoordinatedGraphicsLayer::~CoordinatedGraphicsLayer()
STDERR: 15  0x7ff1f68d61c2 WebCore::CoordinatedGraphicsLayer::~CoordinatedGraphicsLayer()
STDERR: 16  0x7ff1f67baaee void WTF::deleteOwnedPtr<WebCore::GraphicsLayer>(WebCore::GraphicsLayer*)
STDERR: 17  0x7ff1f67b86ef WTF::OwnPtr<WebCore::GraphicsLayer>::~OwnPtr()
STDERR: 18  0x7ff1f68dde79 WebKit::CoordinatedLayerTreeHost::~CoordinatedLayerTreeHost()
STDERR: 19  0x7ff1f68ddf02 WebKit::CoordinatedLayerTreeHost::~CoordinatedLayerTreeHost()
STDERR: 20  0x7ff1f68a44f6 WTF::RefCounted<WebKit::LayerTreeHost>::deref()
STDERR: 21  0x7ff1f68a3e71 void WTF::derefIfNotNull<WebKit::LayerTreeHost>(WebKit::LayerTreeHost*)
STDERR: 22  0x7ff1f68a3b29 WTF::RefPtr<WebKit::LayerTreeHost>::~RefPtr()
STDERR: 23  0x7ff1f68a06fe WebKit::DrawingAreaImpl::~DrawingAreaImpl()
STDERR: 24  0x7ff1f68a0778 WebKit::DrawingAreaImpl::~DrawingAreaImpl()
STDERR: 25  0x7ff1f68c9725 void WTF::deleteOwnedPtr<WebKit::DrawingArea>(WebKit::DrawingArea*)
STDERR: 26  0x7ff1f68ca404 WTF::OwnPtr<WebKit::DrawingArea>::clear()
STDERR: 27  0x7ff1f68c7d88 WTF::OwnPtr<WebKit::DrawingArea>::operator=(std::nullptr_t)
STDERR: 28  0x7ff1f68ba6ac WebKit::WebPage::close()
STDERR: 29  0x7ff1f698ae8b void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)()>(CoreIPC::Arguments0 const&, WebKit::WebPage*, void (WebKit::WebPage::*)())
STDERR: 30  0x7ff1f698970f void CoreIPC::handleMessage<Messages::WebPage::Close, WebKit::WebPage, void (WebKit::WebPage::*)()>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)())
STDERR: 31  0x7ff1f6985423 WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)

[WebKit Review Bot]

Re-opened since this is blocked by bug 104798

[Csaba Osztrogonác]

It caused many crashes on Qt-WK2 pixel tester bot too:
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Pixel%20Tests%29/r137446%20%28875%29/results.html

[Dongseong Hwang]

(In reply to comment #58)
> It caused many crashes on Qt-WK2 pixel tester bot too:
> http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Pixel%20Tests%29/r137446%20%28875%29/results.html

Thank you for reporting. This crash origins from what period CoordinatedGraphicsLayer can communicate via CoordinatedLayerTreeHost.
CoordinatedGraphicsLayer's constructing time and destructing time is vulnerable now.
It is related to Bug 104518.

I'll fix the timing vulnerability and then post this patch again.

[Dongseong Hwang]

(In reply to comment #58)
> It caused many crashes on Qt-WK2 pixel tester bot too:
> http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Pixel%20Tests%29/r137446%20%28875%29/results.html

I think qt crash list is not related to this bug. I also tested qt layout test with debug build before posting this patch. And I also encountered many crash like your list.
notoriously,
crash log for WebKitTestRunner (pid 26230):
STDOUT: <empty>
STDERR: <empty>

(In reply to comment #56)
> This patch is asserting on EFL Debug bots. 400+ new failures.
> 
> crash log for WebProcess (pid <unknown>):
> STDOUT: <empty>
> STDERR: ASSERTION FAILED: m_isFlushingLayerChanges
> STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp(774) : virtual void WebCore::CoordinatedGraphicsLayer::removeTile(uint32_t)
> STDERR: 1   0x7ff1f68d87d8 WebCore::CoordinatedGraphicsLayer::removeTile(unsigned int)

But I absolutely created EFL crash. I don't understand why QT Debug WK2 did not crash like this.

As I mentioned in comment #59, first of all, I remove timing vulnerability in Bug 104518.

[Thiago Marcos P. Santos]

(In reply to comment #60)
> (In reply to comment #58)
> > It caused many crashes on Qt-WK2 pixel tester bot too:
> > http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Pixel%20Tests%29/r137446%20%28875%29/results.html
> 
> I think qt crash list is not related to this bug. I also tested qt layout test with debug build before posting this patch. And I also encountered many crash like your list.
> notoriously,
> crash log for WebKitTestRunner (pid 26230):
> STDOUT: <empty>
> STDERR: <empty>
> 
> (In reply to comment #56)
> > This patch is asserting on EFL Debug bots. 400+ new failures.
> > 
> > crash log for WebProcess (pid <unknown>):
> > STDOUT: <empty>
> > STDERR: ASSERTION FAILED: m_isFlushingLayerChanges
> > STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp(774) : virtual void WebCore::CoordinatedGraphicsLayer::removeTile(uint32_t)
> > STDERR: 1   0x7ff1f68d87d8 WebCore::CoordinatedGraphicsLayer::removeTile(unsigned int)
> 
> But I absolutely created EFL crash. I don't understand why QT Debug WK2 did not crash like this.
> 

AFAIK Qt doesn't have a WK2 Debug bot. The crashes on the release bot won't be assertions but likely to be SEGV's caused by memory corruption.

> As I mentioned in comment #59, first of all, I remove timing vulnerability in Bug 104518.

[Dongseong Hwang]

Created attachment 179223 [details]
Patch

[Dongseong Hwang]

Comment on attachment 179223 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179223&action=review

I re-post this patch because Bug 104798 rolled out this patch, because this patch caused crash.
I tested many sites and layout test with WK2 EFL Debug.

I missed handling ~CoordinatedGraphicsLayer().

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:777
> +    ASSERT(m_coordinator->isFlushingLayerChanges() || m_isPurging);

removeTile can be called during purging also. 

Bug 104880 ensures ~CoordinatedGraphicsLayer() or ~CoordinatedLayerTreeHost() calls purgeBackingStores().
It means removeTile can be called only during flushing or purging.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.h:222
> +    bool m_isPurging;

We only check m_isPurging instead of m_isFlushingCompositingLayer because CoordinatedLayerTreeHost now checks m_isFlushingCompositingLayer.

[WebKit Review Bot]

Comment on attachment 179223 [details]
Patch

Rejecting attachment 179223 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
tedLayerTreeHost.cpp
Hunk #1 succeeded at 614 (offset -4 lines).
Hunk #2 succeeded at 638 (offset -4 lines).
Hunk #3 succeeded at 720 (offset -4 lines).
Hunk #4 succeeded at 760 (offset -4 lines).
patching file Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h
Hunk #1 succeeded at 85 (offset -1 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kenneth Ro..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/15319166

[Dongseong Hwang]

(In reply to comment #64)
> (From update of attachment 179223 [details])
> Rejecting attachment 179223 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> tedLayerTreeHost.cpp
> Hunk #1 succeeded at 614 (offset -4 lines).
> Hunk #2 succeeded at 638 (offset -4 lines).
> Hunk #3 succeeded at 720 (offset -4 lines).
> Hunk #4 succeeded at 760 (offset -4 lines).
> patching file Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h
> Hunk #1 succeeded at 85 (offset -1 lines).
> 
> Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kenneth Ro..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue
> 
> Full output: http://queues.webkit.org/results/15319166

Thank you for review.

This bug depends on Bug 104518 and Bug 104880.

[Dongseong Hwang]

Created attachment 179831 [details]
Patch

[Dongseong Hwang]

thx for light speed cq+ :)

[Dongseong Hwang]

I obsoleted patch because layout test hang up when I rebuilt, although I had tested this patch well.
But I verified this patch does not related to hang up.
So I request cq+ again.

[WebKit Review Bot]

Comment on attachment 179831 [details]
Patch

Clearing flags on attachment: 179831

Committed r137968: <http://trac.webkit.org/changeset/137968>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.