1346501 - we consider every image visible when it is first created

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, enhancement)

Description

[Timothy Nikkel (:tnikkel)]

Bug 1157546 (https://hg.mozilla.org/mozilla-central/rev/2d171d75b746) marks every frame as visible when it is first created. Because of this code https://dxr.mozilla.org/mozilla-central/rev/4ceb9062ea8f4113bfd1b3536ace4a840a72faa7/dom/base/nsImageLoadingContent.cpp#1495

nsImageLoadingContent::FrameCreated is called from the frame's Init() function, which happens before the primary frame pointer on the content is set. It calls nsImageLoadingContent::TrackImage, frame is null (because the primary frame pointer is null at this point), but mFrameCreateCalled is true because FrameCreated just set it. So we don't hit the early exit and consider the image visible.

The code is not doing this intentionally. The code thinks it is doing what it is doing because of feImage (but this is wrong), and it applies to all images.

The commit msgs and comments in the patches explain all this in much more excoriating detail.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Pass a frame to TrackImage when called from FrameCreated so we can access the visibility of the frame

Sorry to dump this review on you Mats, but I don't know of anyone else who has any familiarity with this code.

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: Pass frame to OnVisibilityChange

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Remove now unused variable

Comment 4

[Robert Longson [:longsonr]]

feImageFrames are always marked visible here: https://dxr.mozilla.org/mozilla-central/source/layout/svg/SVGFEImageFrame.cpp#110

so do we need the primary frame passing stuff at all, could we not simply assume primary frame == null means invisible?

Comment 5

[Timothy Nikkel (:tnikkel)]

(In reply to Robert Longson from comment #4)
> so do we need the primary frame passing stuff at all, could we not simply
> assume primary frame == null means invisible?

No, we can't. Because these functions are called from the frame's Init() function the primary frame pointer has not been set yet. So even though we do indeed have a frame our primary frame pointer is null (it gets filled in almost immediately after).

If we genuinely do not have a frame, then yes, we do assume the image is not visible.

Comment 6

[Timothy Nikkel (:tnikkel)]

And yes, I agree that this is a horrible, horrible set of patches.

Comment 7

[Mats Palmgren (inactive)]

nsIFrame::GetVisibility() only returns APPROXIMATELY_NONVISIBLE if the frame's
visibility count > 0.  How can it be non-zero when called from Init()?

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #7)
> nsIFrame::GetVisibility() only returns APPROXIMATELY_NONVISIBLE if the
> frame's
> visibility count > 0.  How can it be non-zero when called from Init()?

I'm assuming you mean APPROXIMATELY_VISIBLE here.

The answer is a little complicated. The "normal" sequence of events is:

-frame is created
-nsImageLoadingContent::FrameCreated called -> image is not visible
-slightly later the frame is reflowed, the reflow callback of nsImageFrame determines if image is visible or not

So we would determine the frame is not visible at frame create time, and that is fine because we will determine at reflow if its visible. So the answer to your question is that in the normal flow we would never have GetVisibility return APPROXIMATELY_VISIBLE during FrameCreate.

However things aren't that simple. SVGFEImageFrame::Init calls IncApproximateVisibleCount() (to make the frame visible) and then calls nsImageLoadingContent::FrameCreated. So this time when FrameCreated is called the frame's GetVisibility returns APPROXIMATELY_VISIBLE.

(Writing this answer I realized the order of these two calls in SVGFEImageFrame::Init was the reverse of what I was thinking, so we don't need the second patch. I'll update them.)

Comment 9

[Timothy Nikkel (:tnikkel)]

Attachment: Pass a frame to TrackImage when called from FrameCreated so we can access the visibility of the frame

Two changes in this new version:
-the commit msg is updated (the last paragraph)
-added a comment to SVGFEImageFrame::Init to keep the ordering of IncApproximateVisibleCount and FrameCreated calls the same

Comment 10

[Mats Palmgren (inactive)]

So the only caller where the TrackImage calls in FrameCreated actually does
anything is SVGFEImageFrame::Init?  Are those TrackImage calls important
for that frame type, or can they wait until we reflow it?

Comment 11

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #10)
> So the only caller where the TrackImage calls in FrameCreated actually does
> anything is SVGFEImageFrame::Init?

With the current setup, yes. When we tracked the visibility on the content node it would survive a reframe so the frame could be visible immediately on frame create. And I'd prefer not to bake assumptions into the code about how the current setup works, because tracking the visibility on the frame has its downsides (this bug being evidence of that), so that we could change/improve how this works in the future.

> Are those TrackImage calls important
> for that frame type, or can they wait until we reflow it?

Yes, I think the TrackImage calls are important. <feImage> is never drawn directly, it is used in a filter that is then applied to another element. So reflowing the SVGFEImageFrame doesn't tell us if it is visible or not. Only nsImageFrame has the reflow callback that determines if it is visible or not; SVGFEImageFrame doesn't have the reflow callback because it would be meaningless to do so.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #11)
> (In reply to Mats Palmgren (:mats) from comment #10)
> > Are those TrackImage calls important
> > for that frame type, or can they wait until we reflow it?
> 
> Yes, I think the TrackImage calls are important. <feImage> is never drawn
> directly, it is used in a filter that is then applied to another element. So
> reflowing the SVGFEImageFrame doesn't tell us if it is visible or not. Only
> nsImageFrame has the reflow callback that determines if it is visible or
> not; SVGFEImageFrame doesn't have the reflow callback because it would be
> meaningless to do so.

I guess we could make the IncApproximateVisibleCount call at a later time, but where exactly? I'm not familiar with svg frames. Are SVGFEImageFrame guaranteed to get a reflow call even though they are "non-display" frames? Or is there another function that we know will be called after Init?

Comment 13

[Robert Longson [:longsonr]]

Non-display frames (which svg images may be and FEImages always are) don't participate in reflow.

https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGFEImageElement.cpp#67 gets called to load the image (may get called multiple times). 

And there's https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGFEImageElement.cpp#157 when it attaches the document.

SVG images can be in patterns so they may need some changes too as images in patterns don't participate in reflow either. https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGImageFrame.cpp#162

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to Robert Longson from comment #13)
> https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGFEImageElement.
> cpp#67 gets called to load the image (may get called multiple times). 

I don't think that'll work as we may not have a frame when it's called.
 
> And there's
> https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGFEImageElement.
> cpp#157 when it attaches the document.

That's too early, we definitely don't have a frame when that is called.

> SVG images can be in patterns so they may need some changes too as images in
> patterns don't participate in reflow either.
> https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGImageFrame.
> cpp#162

Thanks for pointing that out, I didn't know that.

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 8846349 [details] [diff] [review]
Pass a frame to TrackImage when called from FrameCreated so we can access the visibility of the frame

r=mats
(sorry for the delay, I got side-tracked by security bugs)

Comment 16

[Mats Palmgren (inactive)]

BTW, I was looking through the nsImageLoadingContent code and found a potential issue
in nsImageLoadingContent::MakePendingRequestCurrent:
http://searchfox.org/mozilla-central/rev/557f236c19730116d3bf53c0deef36362cafafcd/dom/base/nsImageLoadingContent.cpp#1309
Shouldn't mPendingRequestRegistered be copied to mCurrentRequestRegistered there
and then reset?

Comment 17

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #16)
> BTW, I was looking through the nsImageLoadingContent code and found a
> potential issue
> in nsImageLoadingContent::MakePendingRequestCurrent:
> http://searchfox.org/mozilla-central/rev/
> 557f236c19730116d3bf53c0deef36362cafafcd/dom/base/nsImageLoadingContent.
> cpp#1309
> Shouldn't mPendingRequestRegistered be copied to mCurrentRequestRegistered
> there
> and then reset?

Nice catch! I made this into bug 1348972.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/904ac1dd173e
Don't mark every image as visible when a frame is created for it. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2565ed1ed1
Remove mFrameCreateCalled from nsImageLoadingContent, it is now unused. r=mats

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/904ac1dd173e
https://hg.mozilla.org/mozilla-central/rev/fb2565ed1ed1