this was originally going to be part of bug 435296, but bz preferred to land without it. See https://bugzilla.mozilla.org/show_bug.cgi?id=435296#c17 We need to lock images when the presshell comes in an out of the bfcache, which we can do by hooking into Freeze() and Thaw(). Background tabs is trickier, and it involves fixing bug 343515. I'll talk more with bz about it after bug 435296 lands.

Added a WIP patch. This handles nsImageFrames, but not style images yet (ie, backgrounds). It also doesn't know what the active tab is (bug 343515), so at the moment it just locks/unlocks when things come in and out of the buffer cache. Builds, but completely untested.

Updated WIP. I had to move image tracking from nsImageFrame to nsImageLoadingContent because nsImageFrame doesn't really know enough about when images become relevant and irrelevant. Right now it works ok but there's a weird crash in Thaw() when coming out of the bfcache. I'm hoping to ask roc about it later.

rebased this onto trunk / bug 521497. Untested as of yet, because I'm building right now.

Added a new patch that seems to work. The crash turned out to be the result of the fact that nsImageLoadingContent instances couldn't access their presshell while they were in the bfcache, so they couldn't tell the presshell when their images were going away. bz's solution for this was to move the tracker to the document, which I did. I then had a problem because I cleared the tracker in nsDocument::Destroy(), but various pieces of content tried to unregister themselves between the call to nsDocument::Destroy() and nsDocument's destructor proper (after which point content can't get a pointer to the document anymore). I moved the code to the destructor, which fixed that problem. So now I've got a full stack of patches that fix the flickering issue! My next task is to get this stuff landed. I need to start with getting bug 343515 and bug 521497 reviewed and landed. I also need to take care of bug 579122, which crops up now and then with this patch. And there's bug 580669. Woo! Onward!

Sweet!

This blocks bug 563088 (turning discarding back on), which is a blocker. Requesting blocking.

Comment on attachment 459196 [details] [diff] [review] patch v4 This still might need a couple minor rebasing changes once the final versions of bug 343515 and bug 521497 land, but the code should remain functionally the same. I think it's worth getting roc to look at it now. Flagging for review.

Comment on attachment 459196 [details] [diff] [review] patch v4 + // If the count is now zero, remove from the tracker. + if (count == 0) + mImageTracker.Remove(aImage); + + // Otherwise, set the new value. + else + mImageTracker.Put(aImage, count); Put some {} here to make this all much easier to read + // Whether we're currently holding a lock on all of our images. + PRBool mLockingImages; Make this a bitfield next to some others so it packs + if (NS_SUCCEEDED(rv)) + TrackImage(req); + else { {} PRBool mIsActive; + /* + * Whether we're currently frozen. + */ + PRBool mFrozen; These should be PRPackedBool We need some documentation to explain which images are tracked and why and when they are locked and what that means

What's the story for CSS images?

(In reply to comment #10) > What's the story for CSS images? Whoops, good catch. When I revived this bug 9 months later, I totally forgot about comment 2: > This handles nsImageFrames, but not style images yet (ie, backgrounds). I'm kind of tempted to make CSS images a separate patch and land them incrementally, so that any problems are easier to isolate. Does anyone have an opinion on this?

I'd like to at least know what the plan is for CSS images before we land this.

If we're using mIsActive for this, don't we also need to propagate mIsActive to resource document presshells?

Addressed roc's review comments (still need to look into bz's).

This blocks beta 5, the feature freeze.

There were some linkage problems in the previous patch that didn't show up when I was doing an incremental build. I had to make SetIsActive virtual and stick it in nsPresShell.cpp.

Added a patch to make ImageRenderer take a pointer to nsStyleImage rather than making a copy. This is necessary because we want to assert in nsStyleImage::GetImageData that we've registered the image with the document, but we don't want to waste time registering the short-lived copy that ImageRenderer makes.

Lock nsStyleImages at the end of nsStyleBackground::ComputeBackgroundData, and unlock them in Destroy().

Lock nsStyleContentData images at the end of nsRuleNode::ComputeContentData(), and unlock them in Destroy().

(In reply to comment #13) > If we're using mIsActive for this, don't we also need to propagate mIsActive to > resource document presshells? I filed bug 585129. I'm not sure how common it is to have images in resource documents, so I can't tell whether it should block this bug. Can someone please advise?

Comment on attachment 462952 [details] [diff] [review] patch v6 Also flagging dbaron for sr on the base patch.

Would it make sense to add a listener/observer which makes it possible to listen for changes of mIsActive (and mFrozen) in PresShell instead of calling SetImageLockingState from PresShell::SetIsActive/PresShell::Thaw()? I would need to update active state of plugins for bug 583109 and I guess it would be better to listen to changes from PresShell instead of having PresShell updating all nsObjectFrames.

(In reply to comment #22) > I guess it would be better to listen to changes from PresShell instead of > having PresShell updating all nsObjectFrames. I guess I'm not clear on what the distinction is? All we're doing here is implementing a special-purpose listener/observer architecture. No traversal is done - the relevant pieces of layout code register themselves with the image tracker, and that image tracker "notifies" (via locking/unlocking) those images whenever something interesting happens. The machinery I'm introducing here is special-purpose for sure, and I'd be in favor of making it more general to support things other than images. Still, I want to land it as-is for now though, because I'm pressed for time with the release schedule.

Comment on attachment 463599 [details] [diff] [review] part 2 - v1 - Make ImageRenderer take a pointer to nsStyleImage rather than making a copy r=dbaron

Comment on attachment 463600 [details] [diff] [review] css-story - part 2 - v1 r=dbaron It might be worth making nsStyleBackground::Layer::TrackImages/UntrackImages be inline.

Comment on attachment 463600 [details] [diff] [review] css-story - part 2 - v1 oh, and this could also use a better commit message (e.g., "call nsDocument::AddImage and RemoveImage on images stored in nsStyleBackground").

Are you also planning to do this for the images for 'cursor', 'border-image' and 'list-style-image'? Those should be a good bit easier than the properties you've already done.

Comment on attachment 463601 [details] [diff] [review] part 4 - v1 - call nsDocument::AddImage and RemoveImage on images stored in nsStyleContentData r=dbaron

Comment on attachment 462952 [details] [diff] [review] patch v6 Please rev the IID on nsIDocument. Also, in nsIDocument, document that the image locking state of documents starts out as unlocked/false. sr=dbaron with that

Just to re-emphasize comment 27: these patches look great, but I think you need one more to cover the remaining properties (which should be simpler, although cursor involves a loop).

Updated base patch to address dbaron's sr comments.

Updated background patch to address dbaron's review comments. I think I'm actually going to obsolete the ones with r+, because otherwise things will get too unwieldy.

Adding a patch to handle nsCursorImage. I spoke with dbaron about it on IRC, and he was ok with just locking them unconditionally, since they're unlikely to be large.

Comment on attachment 466548 [details] [diff] [review] part 6 - v1 - lock images in nsStyleList::SetListStyleImage Added what should hopefully be the last patch in this set - locking images unconditionally in nsStyleList.

pushed this to try as 52904383adc8.

No Windows try server builds. Intentional?

Comment on attachment 466547 [details] [diff] [review] part 5 - v1 - make nsCursorImage::mImage private, add getter/setter, and lock images in the setter. You also need to give nsCursorImage a copy-constructor and an assignment operator. (The assignment operator is definitely used in nsStyleUserInterface::CopyCursorArrayFrom .) Otherwise this looks fine, though.

Comment on attachment 466548 [details] [diff] [review] part 6 - v1 - lock images in nsStyleList::SetListStyleImage You also need to fix nsStyleList's copy constructor. Otherwise this looks fine, though.

(In reply to comment #37) > No Windows try server builds. Intentional? Not intentional - looks like on windows you can't define a method with NS_IMETHODIMP if you declare it with NS_HIDDEN_(nsresult). I'll fix that.

Comment on attachment 466716 [details] [diff] [review] part 5 - v2 - make nsCursorImage::mImage private, add getter/setter, and lock images in the setter r=dbaron

Comment on attachment 466717 [details] [diff] [review] part 6 - v2 - lock images in nsStyleList::SetListStyleImage r=dbaron

review is all good to go. I gave this one last push to try (6c83ad835dec) to verify that the windows build problems are fixed, and that we fixed the aborts we were getting without the copy constructor and assignment operator. If those results come back green, I'll land this.

Using that tryserver build (6c83ad835dec), flickering is pretty much gone. The only bug, I notice remains, affects the sidebar and occurs only when decode-on-draw is enabled. It does not appear to be related to discarding, as it does not happen if discarding is enabled but decode-on-draw is disabled. Specifically, random sidebar icons do not update until a mouseover. You see this with history sidebar in "By Last Visited" mode. I suppose that's probably Bug 516320. Tested on Windows XP SP3.

(In reply to comment #50) > Using that tryserver build (6c83ad835dec), flickering is pretty much gone. The > only bug, I notice remains, affects the sidebar and occurs only when > decode-on-draw is enabled. It does not appear to be related to discarding, as > it does not happen if discarding is enabled but decode-on-draw is disabled. > Specifically, random sidebar icons do not update until a mouseover. You see > this with history sidebar in "By Last Visited" mode. I suppose that's probably > Bug 516320. Can you file a bug on that blocks bug 573583?

(In reply to comment #51) > Can you file a bug on that blocks bug 573583? Done

pushed to mozilla-central: part 1: http://hg.mozilla.org/mozilla-central/rev/6a4b70f8944e part 2: http://hg.mozilla.org/mozilla-central/rev/a756c829542a part 3: http://hg.mozilla.org/mozilla-central/rev/fad2fd7c2ad0 part 4: http://hg.mozilla.org/mozilla-central/rev/5e1f15ae17ab part 5: http://hg.mozilla.org/mozilla-central/rev/832a644e8dca part 6: http://hg.mozilla.org/mozilla-central/rev/9594f23591a1 Resolving fixed.

This appears to be causing a crash: bug 588681