Last Comment Bug 683290 - We won't discard any images on the current tab even if they are not in the DOM
: We won't discard any images on the current tab even if they are not in the DOM
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal with 8 votes (vote)
: mozilla17
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
: 713585 765574 (view as bug list)
Depends on: 684919 685516 783379 784481 789800 791731 791779
Blocks: image-suck 679775 693319
  Show dependency treegraph
 
Reported: 2011-08-30 13:26 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-09-17 12:15 PDT (History)
39 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?


Attachments
Patch (47.71 KB, patch)
2011-09-07 07:39 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (46.34 KB, patch)
2011-09-07 13:13 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bzbarsky: review-
Details | Diff | Splinter Review
Patch (48.79 KB, patch)
2011-09-26 06:47 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (now with hg qref) (48.79 KB, patch)
2011-09-26 06:49 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bzbarsky: review+
Details | Diff | Splinter Review
Make RasterImage::GetFrame honor the flags it (1.51 KB, patch)
2011-10-09 14:54 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bobbyholley: review+
Details | Diff | Splinter Review
Patch, again (32.14 KB, patch)
2012-07-20 09:19 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
bzbarsky: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2011-08-30 13:26:07 PDT
Chrome and Safari will discard in this case. We should too. This situation can occur on things like facebook slideshows.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-07 07:39:41 PDT
Created attachment 558801 [details] [diff] [review]
Patch

This builds on the patch in Bug 684919 which adds COMPtrWithFlag<T>, which acts mostly like nsCOMPtr<T> with a flag bit.  The API might change in that bug, so this may end up looking a bit different.

The flag bit is used to store whether or not the image should be tracked.  Then we add a BindToTree/UnbindFromTree implementation that registers/unregisters the image with the document depending on the value of that flag.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-07 09:20:32 PDT
Comment on attachment 558801 [details] [diff] [review]
Patch

Joe says this should be reviewed by a content person.
Comment 3 Joe Drew (not getting mail) 2011-09-07 09:25:51 PDT
Comment on attachment 558801 [details] [diff] [review]
Patch

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

::: content/base/src/nsImageLoadingContent.cpp
@@ +329,5 @@
>  
>    // We can only do this if we have a presshell
> +  // XXXkhuey should this be GetOurCurrentDocument?  Decoding if we're not in
> +  // the document seems silly.
> +  nsIDocument* doc = GetOurOwnerDocument();

When do we have an owner document but not a current document? (This is one of the reasons I want a content person to review this. ;) )

@@ +845,5 @@
> +  // XXXkhuey this is an incredibly nasty hack, but because cloning notifies
> +  // synchronously, we can't use a temporary nsCOMPtr here (we'll assert to
> +  // death in OnStopDecode).  Note that the reference on the outparameter
> +  // is owned by the RequestWithState, so this is not leaky, even though it
> +  // looks like it might be.  Bug 580466 shall save us!

Can you note in that bug so whoever fixes it knows to fix this too?
Comment 4 Boris Zbarsky [:bz] 2011-09-07 09:34:39 PDT
Comment on attachment 558801 [details] [diff] [review]
Patch

Canceling review request for now per irc discussion.  Specifically:

1)  We're going to add a getter_AddRefs that takes a RequestWithState (or
    generally a COMPtrAndFlag, I would think).
2)  Kyle will test whether this gets us flicker on image-swapping tests and if so
    how to deal with that.
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-07 13:13:09 PDT
Created attachment 558922 [details] [diff] [review]
Patch
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-07 13:14:33 PDT
The attached patch addresses point #1 from comment 4.  I've verified that we can get flicker (asynchronous decoding) with this patch, so that remains an issue.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-08 05:04:09 PDT
Comment on attachment 558922 [details] [diff] [review]
Patch

We discussed a possible bandaid for flickering issues on IRC, which is to do some amount of decoding synchronously in RasterImage::RequestDecode, and then do the rest (if any is left) asynchronously on the event loop.  I'm going to file a separate bug for addressing that.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-08 06:03:25 PDT
Note that with the attached patch (and without Bug 685516) we will always do a decode when <img>.src is set and we will always decode synchronously when inserting an image that has a compressed size of less than 150K into the DOM, so the potential for flicker is fairly low to begin with.
Comment 9 Boris Zbarsky [:bz] 2011-09-23 20:55:14 PDT
Comment on attachment 558922 [details] [diff] [review]
Patch

> nsImageLoadingContent::ClearCurrentRequest(nsresult aReason)
>+  mCurrentRequest.Set(nsnull, false);

Why not Clear()?

Similar in ClearPendingRequest.

The comment about why GetOurDocument is not called GetDocument should now explain why the methods are not GetOwnerDoc and GetCurrentDoc.

Speaking of which, probably better to call the new methods GetOurOwnerDoc and GetOurCurrentDoc.  But either way.

You need to add BindToTree and UnbindFromTree calling up into nsImageLoadingContent to nsSVGImageElement.  r- for this part to make sure it happens.
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-26 06:47:59 PDT
Created attachment 562417 [details] [diff] [review]
Patch

Not sure if you wanted to look at this again ...
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-26 06:49:49 PDT
Created attachment 562418 [details] [diff] [review]
Patch (now with hg qref)
Comment 12 Boris Zbarsky [:bz] 2011-09-26 09:47:54 PDT
> Not sure if you wanted to look at this again ...

I did.  Hence the r- vs "r+ if you change this".
Comment 13 Boris Zbarsky [:bz] 2011-09-26 09:48:55 PDT
Comment on attachment 562418 [details] [diff] [review]
Patch (now with hg qref)

r=me
Comment 14 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-09-26 10:35:34 PDT
(In reply to Boris Zbarsky (:bz) from comment #12)
> > Not sure if you wanted to look at this again ...
> 
> I did.  Hence the r- vs "r+ if you change this".

It felt more like an "r- so you pay attention" ;-)

Thanks for the review.
Comment 15 Scott Johnson (:jwir3) 2011-10-07 15:40:02 PDT
Kyle, 

Has this bug landed? It appears to be finished, but isn't closed.
Comment 16 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-08 04:47:33 PDT
There are some test failures I need to sort through before landing.
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-09 14:54:00 PDT
Created attachment 565836 [details] [diff] [review]
Make RasterImage::GetFrame honor the flags it
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-09 14:56:12 PDT
The test failures are because RasterImage::GetFrame ignores the flags it's given if it doesn't already have a decoder.
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2011-10-10 08:41:28 PDT
Comment on attachment 565836 [details] [diff] [review]
Make RasterImage::GetFrame honor the flags it

r=bholley contingent on that followup bug discussed on IRC (factoring out the 3 copy-pasted instances of the 'screwed' code into a helper method).
Comment 20 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-10 09:21:49 PDT
https://hg.mozilla.org/mozilla-central/rev/701372c96a92
https://hg.mozilla.org/mozilla-central/rev/d3b8c0394c5e

Huzzah!
Comment 21 Matt Brubeck (:mbrubeck) 2011-10-10 13:48:48 PDT
Backed out for Android reftest failures:
https://hg.mozilla.org/mozilla-central/rev/29c1738d7e27
Comment 22 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-11 08:22:13 PDT
The test failures are caused by the patch in bug 685516.
Comment 23 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-13 08:03:54 PDT
Since I can't manage to get the tests running on android under a debugger, we have the following options:

1. Land this without Bug 685516.
2. Scope Bug 685516 to affect desktop only (in other words, keep synchronously decoding small images on mobile).
3. Find someone who can actually debug on android to debug the test failures in Bug 685516.
Comment 24 Jo Hermans 2011-12-26 18:16:13 PST
*** Bug 713585 has been marked as a duplicate of this bug. ***
Comment 25 Justin Lebar (not reading bugmail) 2012-06-17 22:17:24 PDT
*** Bug 765574 has been marked as a duplicate of this bug. ***
Comment 26 Steven Young 2012-06-17 23:48:11 PDT
On behalf of Facebook Inc, let me just say that we are prepared to give over one thousand free "pokes" to anyone who fixes this bug or alternatively https://bugzilla.mozilla.org/show_bug.cgi?id=689623. I will also gladly show you Mark Zuckerberg's desk and give you free dinner and cookies as part of a complementary tour of Facebook HQ in Menlo Park.
Comment 27 Justin Lebar (not reading bugmail) 2012-07-15 21:26:21 PDT
This is one of two worst image-related bugs that we're aware of (the other is bug 689623).  Tons of slideshows on the web hit this.  On a memory-constrained mobile device, this is a particularly bad bug, so k90?.
Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-20 09:19:12 PDT
Created attachment 644367 [details] [diff] [review]
Patch, again

Unbitrotted.  The patch is mostly the same.
Comment 29 Boris Zbarsky [:bz] 2012-07-20 11:58:25 PDT
Comment on attachment 644367 [details] [diff] [review]
Patch, again

The indentation in nsNodeUtils.cpp is off, unless this is a diff -w.

It might make sense to put the flags next to mImageBlockingStatus and make the PRUin16 or even PRUint8 to make them pack better.

r=me with that.
Comment 30 HyungGon Baek 2012-07-30 17:40:07 PDT
Is this patch for FF14?

Can I get this patch for FF11?

Thanks,
Comment 31 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-30 17:42:31 PDT
(In reply to HyungGon Baek from comment #30)
> Is this patch for FF14?

This patch is for trunk, more or less.

> Can I get this patch for FF11?

If you do the work, yes.  Otherwise, no.  It's not a good use of my time to backport this to an unsupported version.
Comment 32 Joe Drew (not getting mail) 2012-07-30 19:07:04 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #31)
> > Can I get this patch for FF11?
> 
> If you do the work, yes.  Otherwise, no.  It's not a good use of my time to
> backport this to an unsupported version.

Not to mention entirely insecure. Please upgrade to the latest version of Firefox!
Comment 33 Manoj 2012-08-15 11:55:01 PDT
Kyle, is something holding back the landing of the reviewed patch?
Comment 34 Andrew McCreight [:mccr8] 2012-08-15 11:59:36 PDT
IIRC, there's some work that needs to be done to update the tests that fail for bogus reasons.
Comment 35 Jim Jeffery not reading bug-mail 1/2/11 2012-08-15 12:20:14 PDT
Appears to me its re-landed just today on m-i, should be coming to m-c next merge hopefully, unless it backed out again:

m-i cset: https://hg.mozilla.org/integration/mozilla-inbound/rev/adf31747cf5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/92727400355a

merged to m-c in cset: https://hg.mozilla.org/mozilla-central/rev/a0bddf5fcb91
with followup fix: https://hg.mozilla.org/mozilla-central/rev/d67c02074ced

No idea why the bug has not been updated...
Comment 36 Ed Morley [:emorley] 2012-08-15 12:26:28 PDT
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #35)
> Appears to me its re-landed just today on m-i, should be coming to m-c next
> merge hopefully, unless it backed out again:

It originated from the build-system repo and was merged to m-c first, then inbound.
Comment 37 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-08-15 13:36:15 PDT
I was waiting to make sure it stuck before updating the bug.  I've already landed and backed out twice.

https://hg.mozilla.org/mozilla-central/rev/92727400355a
https://hg.mozilla.org/mozilla-central/rev/adf31747cf5e

And two followups to deal with johns's nsObjectLoadingContent refactoring.

https://hg.mozilla.org/mozilla-central/rev/30ce9f55180f
https://hg.mozilla.org/mozilla-central/rev/91711277a553

Note You need to log in before you can comment on or make changes to this bug.