If GLContextProvider fails to create GLContext, WebRenderAPI is also not created. In this case, CompositorBridgeParent::AllocPWebRenderBridgeParent() crashes now. In this case, gecko should fallback to LayerManagerComposite composite.

If possible we should try to do the GLContext test early, before we set the gfxFeature for webrender.

Reopen this bug since there is still STR to cause crash because of webrender initialization failure. Only my linux pc, it always failed to create 2nd webrender instance. It caused crash during creating 2nd windows. WebRender's Multi-doc support could address this crash, but it is still necessary to handle 2nd window's initialization failure. https://github.com/servo/webrender/pull/1509/files

Comment on attachment 8892781 [details] [diff] [review] patch - Rebuild CompositorSessions if WebRender is disabled Review of attachment 8892781 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/ipc/GPUProcessManager.cpp @@ +424,5 @@ > +GPUProcessManager::DisableWebRender() > +{ > + MOZ_ASSERT(gfx::gfxVars::UseWebRender()); > + if (!gfx::gfxVars::UseWebRender()) { > + } Empty if condition - did you mean to have an early return here? ::: gfx/layers/wr/WebRenderBridgeParent.cpp @@ +528,5 @@ > } > + // Do no handle DPEnd if WebRender is going to be disabled. > + if (!gfxVars::UseWebRender()) { > + return IPC_OK(); > + } Instead of putting these checks on the parent side, can we instead just reset mWrChild to nullptr in the WebRenderLayerManager::Initialize function when if the textureFactorIdentifier isn't LAYERS_WR? I feel like once we start adding these checks on the parent side we'll have to add them to all the methods which gets pretty annoying. Best to not send the messages from the client side at all. Or are there other conditions where the client sends these messages?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7) > Comment on attachment 8892781 [details] [diff] [review] > patch - Rebuild CompositorSessions if WebRender is disabled > > Review of attachment 8892781 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/ipc/GPUProcessManager.cpp > @@ +424,5 @@ > > +GPUProcessManager::DisableWebRender() > > +{ > > + MOZ_ASSERT(gfx::gfxVars::UseWebRender()); > > + if (!gfx::gfxVars::UseWebRender()) { > > + } > > Empty if condition - did you mean to have an early return here? Yes, I forgot to add it. > > ::: gfx/layers/wr/WebRenderBridgeParent.cpp > @@ +528,5 @@ > > } > > + // Do no handle DPEnd if WebRender is going to be disabled. > > + if (!gfxVars::UseWebRender()) { > > + return IPC_OK(); > > + } > > Instead of putting these checks on the parent side, can we instead just > reset mWrChild to nullptr in the WebRenderLayerManager::Initialize function > when if the textureFactorIdentifier isn't LAYERS_WR? I feel like once we > start adding these checks on the parent side we'll have to add them to all > the methods which gets pretty annoying. Best to not send the messages from > the client side at all. Or are there other conditions where the client sends > these messages? WebRenderLayerManager could not handle the problem well, since the problem happen on parent side. value of gfxVars::UseWebRender() was changed true to false during disabling webrender. It could happen at any timing on compositor thread. The problem happen by using gfxVars::UseWebRender(). It seems better to remove gfxVars::UseWebRender() usage on compositor thread.

> It seems better to remove gfxVars::UseWebRender() usage on compositor thread. Correction: It seems better to reduce gfxVars::UseWebRender() usage on compositor thread.

attachment 8893251 [details] [diff] [review] reduced gfxVars::UseWebRender() usage on compositor thread to avoid the problem during disabling WebRender.

(In reply to Sotaro Ikeda [:sotaro] from comment #8) > WebRenderLayerManager could not handle the problem well, since the problem > happen on parent side. value of gfxVars::UseWebRender() was changed true to > false during disabling webrender. It could happen at any timing on > compositor thread. Sorry, I don't really understand this. The gfxVars check happens on the parent side, but the RecvDPEnd etc calls are triggered because the WebRenderLayerManager sends the SendDPEnd message from the content side. So avoiding that send would avoid the checks in RecvDPEnd that you had in your first patch. The other changes (in the compositable code and other places) were fine, I didn't have a problem with those. > The problem happen by using gfxVars::UseWebRender(). It > seems better to remove gfxVars::UseWebRender() usage on compositor thread. This is ok too. Although honestly it seems like gfxVars::UseWebRender should only ever change from true to false at two times: (1) during widget initialization, if we fail to start webrender or (2) if the gpu process resets, in which case all the state in the compositor is thrown away anyway. So I don't think we need to handle the more general case of the gfxvar changing to false at some random time that we can't predict.

Comment on attachment 8893251 [details] [diff] [review] patch - Rebuild CompositorSessions if WebRender is disabled Review of attachment 8893251 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine. I'd prefer if the commit message went into a bit more detail on the "There is a case" part. Can you describe what scenario triggers that case?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12) > > The problem happen by using gfxVars::UseWebRender(). It > > seems better to remove gfxVars::UseWebRender() usage on compositor thread. > > This is ok too. Although honestly it seems like gfxVars::UseWebRender should > only ever change from true to false at two times: (1) during widget > initialization, if we fail to start webrender or (2) if the gpu process > resets, in which case all the state in the compositor is thrown away anyway. > So I don't think we need to handle the more general case of the gfxvar > changing to false at some random time that we can't predict. If e10s is enabled, but no-gpu process(default on linux and mac), gfxVars::UseWebRender change is soon notified by compositor thread. If WebRender creation failure happens at 2nd WebRender creation, there could be several WebRenderBridgeParents for 1st WebRender. WebRenderBridgeParents could receive several IPC messages that were sent before the gfxVars::UseWebRender change until Rebuild InProcessSessions and LayerManagers in content processes.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12) > (In reply to Sotaro Ikeda [:sotaro] from comment #8) > > WebRenderLayerManager could not handle the problem well, since the problem > > happen on parent side. value of gfxVars::UseWebRender() was changed true to > > false during disabling webrender. It could happen at any timing on > > compositor thread. > > Sorry, I don't really understand this. The gfxVars check happens on the > parent side, but the RecvDPEnd etc calls are triggered because the > WebRenderLayerManager sends the SendDPEnd message from the content side. So > avoiding that send would avoid the checks in RecvDPEnd that you had in your > first patch. The other changes (in the compositable code and other places) > were fine, I didn't have a problem with those. IPC messages from WebRenderLayerManager are basically async, then there is a chance that WebRenderBridgeParents receive the messages after the gfxVars::UseWebRender change. And the gfxVars::UseWebRender change in content process could be delayed than WebRenderBridgeParents.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #13) > Comment on attachment 8893251 [details] [diff] [review] > patch - Rebuild CompositorSessions if WebRender is disabled > > Review of attachment 8893251 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks fine. I'd prefer if the commit message went into a bit more > detail on the "There is a case" part. Can you describe what scenario > triggers that case? Yes, I will add more messages to the commit message.

Pushed by sikeda@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/901c59d3ab9f Rebuild CompositorSessions if WebRender is disabled r=kats