784591 - don't leave around decoded image data for display: none images

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: part 1

Comment 2

[Timothy Nikkel (:tnikkel)]

Attachment: part 2

Comment 3

[Timothy Nikkel (:tnikkel)]

These cause 1 failure on try server that I still need to solve.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1. Make the immediate discard when untracking an image on nsDocument optional.

display: none elements do not get frames created. But frames are also destroyed and quickly recreated if we need to rebuild the frame due to some DOM change that we can't handle any other way. In this case I don't think we want to immediately discard the decoded data. So this adds the ability to do that.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Don't track images that don't have a frame created.

Guessing at reviewer here, please redirect if there is a better choice.

This is perhaps a bit complicated. Open to suggestions to streamline.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 655096 [details] [diff] [review]
Part 1. Make the immediate discard when untracking an image on nsDocument optional.

Review of attachment 655096 [details] [diff] [review]:
-----------------------------------------------------------------

I think that we want to stop doing RequestDiscard entirely for some of the other pieces of fallout from the last image changes.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 655096 [details] [diff] [review]
Part 1. Make the immediate discard when untracking an image on nsDocument optional.

Review of attachment 655096 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, after talking with Justin, we will want the ability to specify whether we discard immediately or not.  We actually will want to backport this to aurora.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 655098 [details] [diff] [review]
Part 2. Don't track images that don't have a frame created.

Review of attachment 655098 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsImageLoadingContent.cpp
@@ +458,5 @@
> +    if (doc) {
> +      mPendingRequestFlags |= REQUEST_IS_TRACKED;
> +      doc->AddImage(mPendingRequest);
> +    }
> +  }

Why don't you just call TrackImage here?  Why should we ever have a frame but not have REQUEST_SHOULD_BE_TRACKED?

::: content/base/src/nsImageLoadingContent.h
@@ +320,5 @@
>    uint32_t mCurrentRequestFlags;
>    uint32_t mPendingRequestFlags;
>  
>    enum {
>      // Set if the request needs 

While you're here, can you add the missing words here?

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #9)
> ::: content/base/src/nsImageLoadingContent.cpp
> @@ +458,5 @@
> > +    if (doc) {
> > +      mPendingRequestFlags |= REQUEST_IS_TRACKED;
> > +      doc->AddImage(mPendingRequest);
> > +    }
> > +  }
> 
> Why don't you just call TrackImage here?  Why should we ever have a frame
> but not have REQUEST_SHOULD_BE_TRACKED?

Can we get rid of the REQUEST_SHOULD_BE_TRACKED bit and assume that all non-null mCurrent/PendingRequest should be tracked?

Comment 11

[Timothy Nikkel (:tnikkel)]

Pushed part 1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f127f704815c

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 655096 [details] [diff] [review]
Part 1. Make the immediate discard when untracking an image on nsDocument optional.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this patch should have no effect. It adds an option and never uses it (so that future patches can use it). Kyle wants this patch so he can write a fix on top of it for bug 791731.
User impact if declined: none
Testing completed (on m-c, etc.): none
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f127f704815c

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In the future, put [leave open] in the whiteboard to avoid auto-resolves like that)

Comment 15

[Justin Wood (:Callek)]

Comment on attachment 655096 [details] [diff] [review]
Part 1. Make the immediate discard when untracking an image on nsDocument optional.

In doing approval-but-not-landed triage:
$ hg qpush
applying discardoptional
patching file content/base/src/nsImageLoadingContent.cpp
Hunk #1 FAILED at 1167
1 out of 2 hunks FAILED -- saving rejects to file content/base/src/nsImageLoadingContent.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh discardoptional

So not pushing (since I don't want to manually unwrap patch rejects right now)

Comment 16

[Justin Lebar (not reading bugmail)]

> $ hg transplant -s ~/code/moz/ff-clean/src f127f704815c

Worked fine for me.

Pushed part 1 to Aurora (needed for bug 791731): https://hg.mozilla.org/releases/mozilla-aurora/rev/c0a393d6210a

Comment 17

[Timothy Nikkel (:tnikkel)]

Kyle, did you see comment 10?

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Timothy Nikkel (:tn) from comment #10)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #9)
> > ::: content/base/src/nsImageLoadingContent.cpp
> > @@ +458,5 @@
> > > +    if (doc) {
> > > +      mPendingRequestFlags |= REQUEST_IS_TRACKED;
> > > +      doc->AddImage(mPendingRequest);
> > > +    }
> > > +  }
> > 
> > Why don't you just call TrackImage here?  Why should we ever have a frame
> > but not have REQUEST_SHOULD_BE_TRACKED?
> 
> Can we get rid of the REQUEST_SHOULD_BE_TRACKED bit and assume that all
> non-null mCurrent/PendingRequest should be tracked?

I believe we can.  The everytime we call PrepareNextRequest and store something into it we then track the request.

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #9)
> ::: content/base/src/nsImageLoadingContent.cpp
> @@ +458,5 @@
> > +    if (doc) {
> > +      mPendingRequestFlags |= REQUEST_IS_TRACKED;
> > +      doc->AddImage(mPendingRequest);
> > +    }
> > +  }
> 
> Why don't you just call TrackImage here?  Why should we ever have a frame
> but not have REQUEST_SHOULD_BE_TRACKED?

We actually can't call TrackImage from FrameCreated because the primary frame pointer on the content node hasn't been setup when FrameCreated is called so there isn't a way for TrackImage to tell that we have a frame.

Comment 20

[Timothy Nikkel (:tnikkel)]

Attachment: Part 1.5. Remove the SHOULD_BE_TRACKED bit.

Comment 21

[Timothy Nikkel (:tnikkel)]

Comment on attachment 699037 [details] [diff] [review]
Part 1.5. Remove the SHOULD_BE_TRACKED bit.

I think Kyle might be busy, trying another reviewer, not sure if good choice.

Comment 22

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Don't track images that don't have a frame created.

Rebased on top of part 1.5.

Comment 23

[Joe Drew (not getting mail)]

Comment on attachment 699039 [details] [diff] [review]
Part 2. Don't track images that don't have a frame created.

Review of attachment 699039 [details] [diff] [review]:
-----------------------------------------------------------------

This is all sort of over my head. Maybe jlebar?

Comment 24

[Justin Lebar (not reading bugmail)]

Comment on attachment 699039 [details] [diff] [review]
Part 2. Don't track images that don't have a frame created.

I think I can look at this, but are you OK waiting a few days?  I'm at the B2G work week.

Comment 25

[Timothy Nikkel (:tnikkel)]

Sure, a few days is better than 3 months. Alternatively I'm not sure who are all good reviewers here, so feel free to suggest another.

Comment 26

[Justin Lebar (not reading bugmail)]

Comment on attachment 699039 [details] [diff] [review]
Part 2. Don't track images that don't have a frame created.

Sorry for making you wait so long on this one; thanks for being patient.

Can mPendingRequest == mCurrentRequest?  It's not totally clear to me from the existing code, but perhaps we should at least assert not, because you assume not in TrackImage.  Or if you're not confident in that assumption, let's just make the code work in both cases.

What if we encapsulated this better?  e.g.

  EnsureRequestIsTrackedInDoc(imgIRequest* aRequest, nsIDocument* aDocument)
  {
    MOZ_ASSERT(!aRequest || aRequest == mCurrentRequest || aRequest == mPendingRequest);

    if (!aRequest) {
      return;
    }

    if (aRequest == mCurrentRequest && !(mCurrentRequestFlags & REQUEST_IS_TRACKED)) {
      mCurrentRequestFlags |= REQUEST_IS_TRACKED;
      aDocument->AddImage(mCurrentRequest);
    }

    if (aRequest == mPendingRequest && !(mPendingRequestFlags & REQUEST_IS_TRACKED)) {
      ...
    }
  }

  EnsureRequestNotTrackedInDoc(...) { ... }

Or, if those method names are too similar to each other, here are some
alternatives:

  TrackRequestInDoc / UntrackRequestInDoc, or
  AddRequestToDoc / RemoveRequestFromDoc
  Either of the two above with "Maybe" added on. 

I'm not totally satisfied with any of these names, but the encapsulation feels
much less error-prone to me.  What do you think?

Comment 27

[Timothy Nikkel (:tnikkel)]

(In reply to Justin Lebar [:jlebar] from comment #26)
> Can mPendingRequest == mCurrentRequest?  It's not totally clear to me from
> the existing code, but perhaps we should at least assert not, because you
> assume not in TrackImage.  Or if you're not confident in that assumption,
> let's just make the code work in both cases.

Part 1.5 removes code that has been there for a while that makes the same assumption. But I'm not sure if mPendingRequest == mCurrentRequest is valid or not so I'll make a change like you suggest.

Comment 28

[Timothy Nikkel (:tnikkel)]

Attachment: Part 2. Don't track images that don't have a frame created.

Comment 29

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/304ee1c25362
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d95a6900a4f

Comment 30

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/31fea3e0be16 - Android crashtest-3 said, entirely too often, "ownerdiscard.html | load failed: timed out waiting for reftest-wait to be removed." I gathered up links to the logs that failed in a neat little bundle for you in bug 832571.

Comment 31

[Timothy Nikkel (:tnikkel)]

Attachment: Fix ownerdiscard test.

So it appears the goal of this test is to hit imgRequestProxy::ChangeOwner with mCanceled == true on the img element in the child iframe and then discard that image. This is how that happens.

When the img element gets parsed and added to the tree it will call HTMLImageElement::MaybePreloadImage which will load the image and create a nsInputStreamReadyEvent.

The img element and the iframe containing the other img element are removed from the document via the adoptNode call. This calls Cancel on the imgRequestProxy and sets mCanceled to true.

The nsInputStreamReadyEvent now runs which eventually calls imgRequestProxy::ChangeOwner.

So since it is the img element in the child iframe which is important, waiting for DOMContentLoaded on the parent document isn't good enough, the img element in the child iframe may not have been created yet. So instead wait for DOMContentLoaded on the child iframe to remove the nodes from the document. This seems to test the same condition.

This also seems to make us get the same blockonload assertion more, probably cause we get it for both img elements.

Comment 32

[Josh Matthews [:jdm]]

Comment on attachment 710271 [details] [diff] [review]
Fix ownerdiscard test.

Review of attachment 710271 [details] [diff] [review]:
-----------------------------------------------------------------

Good detective work. Sorry for making you do it!

Comment 33

[Timothy Nikkel (:tnikkel)]

Attachment: disable ownerdiscard on crashtest-ipc

So the fixed test plus the patches from this bug make the test go orange on linux crashtest-ipc some of the time (some pushes seem unaffected, other are not). I've have put in significant effort into reproducing this locally, trying different configurations on three different machines, but so far I have failed to reproduce locally. try is very backed up this whole week on linux32 test jobs. You basically get one result per 24 hours, or two if you are lucky. Making figuring this out via try server much more difficult than usual.

So given that the test is buggy (nothing in the test requires us to ever decode an image) I would like to disable the test on crashtest-ipc so I can unblock on landing these patches while I continue to slowly debug this test via tryserver.

Comment 34

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2fda338874d

Comment 35

[Ed Morley [:emorley]]

Backed out for causing bug 832571 again:
https://tbpl.mozilla.org/php/getParsedLog.php?id=19524181&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/8987eff12bd8

Comment 36

[Timothy Nikkel (:tnikkel)]

Attachment: if we don't decode the image just move on

So before the patches of this bug a full decode of the image could be kicked off either in nsImageLoadingContent::BindToTree when the image was bound to the tree, or through the following sequence:
-HTMLImageElement::MaybeLoadImage is called when the image element is added to the tree. It starts loading the image by creating a network request for it.
-when this network request actually starts getting data we create a RasterImage
-the loading of the image via MaybeLoadImage also calls imgStatusTracker::Notify, which creates a runnable that calls SyncNotify when it runs.
-when SyncNotify is called if the imgStatusTracker has an mImage it calls FrameRect on that image, which asks for a full decode via GetCurrentImgFrame, GetImgFrame, WantDecodedFrames, StartDecoding.
So you can see that if the network request gets data available after the runnable calls imgStatusTracker::SyncNotify we won't have an mImage in SyncNotify.

If our goal is to have an image discarded that has had ChangeOwner called on it's proxy when that proxy had mCanceled == true I think the test is currently do that the best way possible, unless we add some new notifications. The test as it is written has no guarantee that the image will ever be decoded (it's removed before it is ever displayed). And I don't know of a way to force that and maintain the property that we call ChangeOwner on a canceled proxy, because ChangeOwner happens fairly early in the loading of the image, and decoding happens comparatively later.

So given that most of the time we are testing the correct thing by the test, and some of the time don't test anything I think it's reasonable to just move on past the test if the image wasn't decoded.

Comment 37

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1c037c0e1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb62e8cc4ff1
https://hg.mozilla.org/integration/mozilla-inbound/rev/7559ef8dc1bb

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ee1c037c0e1c
https://hg.mozilla.org/mozilla-central/rev/eb62e8cc4ff1
https://hg.mozilla.org/mozilla-central/rev/7559ef8dc1bb

Comment 39

[Timothy Nikkel (:tnikkel)]

Comment on attachment 703194 [details] [diff] [review]
Part 2. Don't track images that don't have a frame created.

We want these to fix bug 843213, which was caused by bug 791571.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 791571 caused bug 843213
User impact if declined: image doesn't show up in some situations. I haven't debugged the problematic page so I'm not sure exactly what the situation is.
Testing completed (on m-c, etc.): three or more weeks on m-c and now on aurora
Risk to taking this patch (and alternatives if risky): there is a small amount of risk with these patches. An alternative would be to invest more time into debugging the problem and fixing it differently; fixing bug 843213 was not a known or sought outcome of landing these patches.
String or UUID changes made by this patch: none

Comment 40

[Timothy Nikkel (:tnikkel)]

Comment on attachment 699037 [details] [diff] [review]
Part 1.5. Remove the SHOULD_BE_TRACKED bit.

[Approval Request Comment]
See comment 39.

Comment 41

[Timothy Nikkel (:tnikkel)]

Comment on attachment 713164 [details] [diff] [review]
if we don't decode the image just move on

[Approval Request Comment]
See comment 39.

Comment 42

[Lukas Blakk [:lsblakk] use ?needinfo]

Timothy, bug 843213 has not landed on FF20 (Beta) - are you intending this for mozilla-aurora approval?

Comment 43

[Timothy Nikkel (:tnikkel)]

(In reply to Lukas Blakk [:lsblakk] from comment #42)
> Timothy, bug 843213 has not landed on FF20 (Beta) - are you intending this
> for mozilla-aurora approval?

Bug 843213 has no patches, it hasn't landed anywhere. This bug (bug 784591) fixes bug 843213. Bug 843213 was caused by bug 791571.

The patches on this bug are already on aurora.

Comment 44

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 699037 [details] [diff] [review]
Part 1.5. Remove the SHOULD_BE_TRACKED bit.

Thanks for the clarification - but looking closer I think that as mentioned in bug 843213 comment 7 - this regression has been around since 17 and since we have the fix in Aurora it can ride the trains from there so we're not taking unecessary risk in Beta at this point.