Directly composited images are images: ASSERT that

Directly composited images are images: ASSERT that

isDirectlyCompositedImage only applies to images. ASSERT that and
make callers do all the isImage() testing.

BUG=405803, 406418
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180721

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180812

[Ian Vollick, 2014-08-21 03:43:29]

It looks like there are some behavioral changes in addition to adding ASSERTs
and calling isDirectlyCompositedImage only on images that I'm a bit concerned
about (comments below), but this seems pretty reasonable. And as you'd
mentioned, this could use a test.

https://codereview.chromium.org/491773002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/491773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:464:
m_graphicsLayer->setContentsToPlatformLayer(layer);
IIUC, it looks like previously, we could still execute one of the following
conditional blocks, even if renderer->isImage() was true, but now we won't. Is
that true (and is this correct)?

https://codereview.chromium.org/491773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1702: return
!isDirectlyCompositedImage();
Similarly, it looks like we used to be possible to execute the rest of this
function when renderer()->isImage().

[Noel Gordon, 2014-08-21 05:39:31]

On 2014/08/21 03:43:29, vollick wrote:
> It looks like there are some behavioral changes in addition to adding ASSERTs
> and calling isDirectlyCompositedImage only on images that I'm a bit concerned
> about (comments below), but this seems pretty reasonable.

I had the same concerns.

> And as you'd  mentioned, this could use a test.

That'll be in the next patch when I fix the actual bug.  This is preparatory
patch which should not change our test results at all, or our behavior.  Let's
examine that.

[Noel Gordon, 2014-08-21 06:16:19]

https://codereview.chromium.org/491773002/diff/1/Source/core/rendering/compos...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/491773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:464:
m_graphicsLayer->setContentsToPlatformLayer(layer);
On 2014/08/21 03:43:29, vollick wrote:
> IIUC, it looks like previously, we could still execute one of the following
> conditional blocks, even if renderer->isImage() was true, but now we won't. Is
> that true (and is this correct)?

Good point, and I wonder which of them would be entered if isImage() was true?

I believe isImage() is true for RenderImage, StyleGeneratedImage, and
RenderListMarker when list's marker is an image.  Given that:

   if ( .... platformLayerForPlugin(renderer)
     plugins are not isImage(), right?

  if (renderer->node() && renderer->node()->isFrameOwnerElement()
     nope frame owners are not isImage(), right?

  if (renderer->isVideo())
     nope, isVideo() -> isMedia() and MediaElement isImage() returns false.

  if (isAcceleratedCanvas(renderer))
     nope, canvas is not isImage() accelerated or not.

concur?

https://codereview.chromium.org/491773002/diff/1/Source/core/rendering/compos...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1702: return
!isDirectlyCompositedImage();
On 2014/08/21 03:43:29, vollick wrote:
> Similarly, it looks like we used to be possible to execute the rest of this
> function when renderer()->isImage().

Originally had this as:

  if (renderer()->isImage() && isDirectlyCompositedImage())
    return false;

and maybe we should do that?

[Noel Gordon, 2014-08-21 06:50:12]

> me said
> and maybe we should do that?

and me replied: yes we should, I don't quite understand the patch that added the
follow-on code or why it was needed. /me going conservative.

[Noel Gordon, 2014-08-21 07:04:43]

On 2014/08/21 03:43:29, vollick wrote:
> It looks like there are some behavioral changes 

Thanks, and agree for the code after line 1704.   New patch uploaded.

[Ian Vollick, 2014-08-21 12:56:46]

On 2014/08/21 at 07:04:43, noel wrote:
> On 2014/08/21 03:43:29, vollick wrote:
> > It looks like there are some behavioral changes 
> 
> Thanks, and agree for the code after line 1704.   New patch uploaded.

Cool, this seems like a harmless refactor now. Does this patch fix an issue with
the test uploaded to the bug? If so, could it be turned into a layout test and
added to this CL?

[Noel Gordon, 2014-08-21 13:03:39]

On 2014/08/21 12:56:46, vollick wrote:
> On 2014/08/21 at 07:04:43, noel wrote:
> > On 2014/08/21 03:43:29, vollick wrote:
> > > It looks like there are some behavioral changes 
> > 
> > Thanks, and agree for the code after line 1704.   New patch uploaded.
> 
> Cool, this seems like a harmless refactor now. Does this patch fix an issue
with
> the test uploaded to the bug? If so, could it be turned into a layout test and
> added to this CL?

Nope re fixing the bug, this patch is a preparation.  A new patch to come fixes
the bug and includes a layout test based on the one I uploaded to the bug.
Sounds good?

[Ian Vollick, 2014-08-21 13:05:22]

On 2014/08/21 at 13:03:39, noel wrote:
> On 2014/08/21 12:56:46, vollick wrote:
> > On 2014/08/21 at 07:04:43, noel wrote:
> > > On 2014/08/21 03:43:29, vollick wrote:
> > > > It looks like there are some behavioral changes 
> > > 
> > > Thanks, and agree for the code after line 1704.   New patch uploaded.
> > 
> > Cool, this seems like a harmless refactor now. Does this patch fix an issue
with
> > the test uploaded to the bug? If so, could it be turned into a layout test
and
> > added to this CL?
> 
> Nope re fixing the bug, this patch is a preparation.  A new patch to come
fixes the bug and includes a layout test based on the one I uploaded to the bug.
Sounds good?

Yup, lgtm.

[Ian Vollick, 2014-08-21 13:05:31]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2014-08-21 13:06:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noel@chromium.org/491773002/20001

[commit-bot: I haz the power, 2014-08-21 16:32:37]

Committed patchset #2 (20001) as 180721

[benm (inactive), 2014-08-22 15:29:33]

this is causing random crashes on android cq after rolling into chromium

https://codereview.chromium.org/491773002/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/491773002/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1765: if
((changeType == ImageChanged) && isDirectlyCompositedImage()) {
looks like you missed a callsite?

[jabdelmalek, 2014-08-22 15:35:18]

A revert of this CL (patchset #2) has been created in
https://codereview.chromium.org/497143002/ by jabdelmalek@google.com.

The reason for reverting is: Breaking android bots most of the time.

[Noel Gordon, 2014-08-22 16:20:57]

https://codereview.chromium.org/491773002/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/491773002/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1765: if
((changeType == ImageChanged) && isDirectlyCompositedImage()) {
On 2014/08/22 15:29:33, benm wrote:
> looks like you missed a callsite?

Thanks and possible, I had checked that this is called by renderer()->isImage()
things. Was there a stack trace or url to the crash btw?

[benm (inactive), 2014-08-22 16:23:34]

Here's one:
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...

The stack is:

signal 11 (SIGSEGV) at 0xfbadbeef (code=1), thread 6124 (Chrome_InProcRe)
pid: 6084, tid: 6124, name: Chrome_InProcRe  >>>
org.chromium.android_webview.shell <<<
signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr fbadbeef
     r0 7addff80  r1 fffffffe  r2 7fa5d9d8  r3 fbadbeef
     r4 3b060970  r5 00000000  r6 00000001  r7 00000001
     r8 00000001  r9 781af74c  sl 7afa97a0  fp 00000000
     ip 17ec4001  sp 7ade0020  lr 7658897d  pc 7658897e

Stack Trace:
  RELADDR   FUNCTION



                                                            FILE:LINE
  0149097e  blink::CompositedLayerMapping::isDirectlyCompositedImage()
const+50


                                                                 libgcc2.c:0
  01492c87
 blink::CompositedLayerMapping::contentChanged(blink::ContentChangeType)+6



                                                 libgcc2.c:0
  014448c1  blink::RenderLayer::contentChanged(blink::ContentChangeType)+88



                                                           libgcc2.c:0
  012ec933  blink::Resource::didAddClient(blink::ResourceClient*)+30



                                                            libgcc2.c:0
  012e9a6b  blink::ImageResource::didAddClient(blink::ResourceClient*)+114



                                                            libgcc2.c:0
  012eff09  blink::Resource::addClient(blink::ResourceClient*)+20



                                                           libgcc2.c:0
  0143cf07
 blink::RenderImageResource::setImageResource(blink::ImageResource*)+86



                                                libgcc2.c:0
  01388d45  blink::ImageLoader::notifyFinished(blink::Resource*)+112



                                                            libgcc2.c:0
  0114bbc3  blink::HTMLImageLoader::notifyFinished(blink::Resource*)+22



                                                           libgcc2.c:0
  012ee3d5  blink::Resource::checkNotify()+66



                                                           libgcc2.c:0
  012ed8d7  blink::Resource::error(blink::Resource::Status)+130



                                                           libgcc2.c:0
  012e9c99  blink::ImageResource::error(blink::Resource::Status)+14



                                                           libgcc2.c:0
  012e9d83  blink::ImageResource::updateImage(bool)+218



                                                           libgcc2.c:0
  012e9dfd  blink::ImageResource::finishOnePart()+18



                                                            libgcc2.c:0
  012ecb61  blink::Resource::finish(double)+112



                                                           libgcc2.c:0
  012f9835  blink::ResourceLoader::didFinishLoading(blink::WebURLLoader*,
double, long long)+188


                                                              libgcc2.c:0
  0194966b  content::WebURLLoaderImpl::Context::OnCompletedRequest(int,
bool, bool, std::string const&, base::TimeTicks const&, long long)+218


                                                                libgcc2.c:0
  0193d011  content::ResourceDispatcher::OnRequestComplete(int,
ResourceMsg_RequestCompleteData const&)+220



 libgcc2.c:0
  0193d897  content::ResourceDispatcher::DispatchMessage(IPC::Message
const&)+802


                                                                 libgcc2.c:0
  0193daa7  content::ResourceDispatcher::OnMessageReceived(IPC::Message
const&)+278


                                                               libgcc2.c:0
  01921b5b  content::ChildThread::OnMessageReceived(IPC::Message const&)+82



                                                           libgcc2.c:0
  0079562d  IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message
const&)+300


                                                                libgcc2.c:0
  00793ab5  base::internal::Invoker<2,
base::internal::BindState<base::internal::RunnableAdapter<void
(IPC::ChannelProxy::Context::*)(IPC::Message const&)>, void
(IPC::ChannelProxy::Context*, IPC::Message const&), void
(IPC::ChannelProxy::Context*, IPC::Message)>, void
(IPC::ChannelProxy::Context*, IPC::Message
const&)>::Run(base::internal::BindStateBase*)+46  libgcc2.c:0
  003062c3  base::debug::TaskAnnotator::RunTask(char const*, char const*,
base::PendingTask const&)+418


                                                             libgcc2.c:0
  00324b09  base::MessageLoop::RunTask(base::PendingTask const&)+140



                                                            libgcc2.c:0
  00324b89  base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
const&)+28


                                                                 libgcc2.c:0
  0032607d  base::MessageLoop::DoWork()+98



                                                            libgcc2.c:0
  003263e5  base::MessagePumpDefault::Run(base::MessagePump::Delegate*)+76



                                                            libgcc2.c:0
  0032578b  base::MessageLoop::RunHandler()+82



                                                            libgcc2.c:0
  00334a9d  base::RunLoop::Run()+12



                                                           libgcc2.c:0
  00324445  base::MessageLoop::Run()+12



                                                           libgcc2.c:0
  0034530b  base::Thread::ThreadMain()+438


On 22 August 2014 17:20, <noel@chromium.org> wrote:

>
> https://codereview.chromium.org/491773002/diff/20001/
> Source/core/rendering/compositing/CompositedLayerMapping.cpp
> File Source/core/rendering/compositing/CompositedLayerMapping.cpp
> (right):
>
> https://codereview.chromium.org/491773002/diff/20001/
> Source/core/rendering/compositing/CompositedLayerMapping.cpp#newcode1765
> Source/core/rendering/compositing/CompositedLayerMapping.cpp:1765: if
> ((changeType == ImageChanged) && isDirectlyCompositedImage()) {
> On 2014/08/22 15:29:33, benm wrote:
>
>> looks like you missed a callsite?
>>
>
> Thanks and possible, I had checked that this is called by
> renderer()->isImage() things. Was there a stack trace or url to the
> crash btw?
>
> https://codereview.chromium.org/491773002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Noel Gordon, 2014-08-22 16:26:08]

Thanks Ben, we'll look into it.

[Noel Gordon, 2014-08-22 16:46:48]

https://codereview.chromium.org/491773002/diff/20001/Source/core/rendering/co...
File Source/core/rendering/compositing/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/491773002/diff/20001/Source/core/rendering/co...
Source/core/rendering/compositing/CompositedLayerMapping.cpp:1765: if
((changeType == ImageChanged) && isDirectlyCompositedImage()) {
On 2014/08/22 16:20:57, Noel Gordon wrote:
> On 2014/08/22 15:29:33, benm wrote:
> > looks like you missed a callsite?
> 
> Thanks and possible, I had checked that this is called by
renderer()->isImage()
> things. Was there a stack trace or url to the crash btw?

Looks to be the poster image of a renderVideo::renderMeadia(). Thanks Ben, will
fix that tomorrow.

[Noel Gordon, 2014-08-23 03:23:55]

Green bubbles and isDirectlyCompositedImage() is prefixed by
renderer()->isImage() everywhere. CQ+

[Noel Gordon, 2014-08-23 03:23:59]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2014-08-23 03:24:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/noel@chromium.org/491773002/40001

[commit-bot: I haz the power, 2014-08-24 04:46:20]

Committed patchset #3 (40001) as 180812