we consider every image visible when it is first created

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout: Images
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8846259 [details] [diff] [review]
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.
Attachment #8846259 - Flags: review?(mats)
(Assignee)

Comment 2

a year ago
Created attachment 8846260 [details] [diff] [review]
Pass frame to OnVisibilityChange
Attachment #8846260 - Flags: review?(mats)
(Assignee)

Comment 3

a year ago
Created attachment 8846261 [details] [diff] [review]
Remove now unused variable
Attachment #8846261 - Flags: review?(mats)
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?
(Assignee)

Comment 5

a year ago
(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.
(Assignee)

Comment 6

a year ago
And yes, I agree that this is a horrible, horrible set of patches.
nsIFrame::GetVisibility() only returns APPROXIMATELY_NONVISIBLE if the frame's
visibility count > 0.  How can it be non-zero when called from Init()?
Flags: needinfo?(tnikkel)
(Assignee)

Comment 8

a year ago
(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.)
Flags: needinfo?(tnikkel)
(Assignee)

Updated

a year ago
Attachment #8846260 - Attachment is obsolete: true
Attachment #8846260 - Flags: review?(mats)
(Assignee)

Comment 9

a year ago
Created attachment 8846349 [details] [diff] [review]
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
Attachment #8846259 - Attachment is obsolete: true
Attachment #8846259 - Flags: review?(mats)
Attachment #8846349 - Flags: review?(mats)
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?
Flags: needinfo?(tnikkel)
(Assignee)

Comment 11

a year ago
(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.
Flags: needinfo?(tnikkel)
(Assignee)

Comment 12

a year ago
(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?
Flags: needinfo?(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
Flags: needinfo?(longsonr)
(Assignee)

Comment 14

a year ago
(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 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)
Attachment #8846349 - Flags: review?(mats) → review+
Attachment #8846261 - Flags: review?(mats) → review+
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?
Flags: needinfo?(tnikkel)
(Assignee)

Comment 17

a year ago
(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.
Flags: needinfo?(tnikkel)

Comment 18

a year ago
Pushed by tnikkel@gmail.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/904ac1dd173e
https://hg.mozilla.org/mozilla-central/rev/fb2565ed1ed1
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.