Closed Bug 512260 Opened 10 years ago Closed 9 years ago

Active PresShells should lock all descendant images to prevent discarding

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(6 files, 13 obsolete files)

13.48 KB, patch
bholley
: review+
bholley
: superreview+
Details | Diff | Splinter Review
5.63 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.14 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.41 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.54 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.83 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
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.
Duplicate of this bug: 516290
Blocks: 435296
No longer depends on: 435296
Attached patch WIP 1 (obsolete) — Splinter Review
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.
OS: Mac OS X → All
Hardware: x86 → All
Attached patch WIP 2 (obsolete) — Splinter Review
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.
Attachment #401771 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
rebased this onto trunk / bug 521497. Untested as of yet, because I'm building right now.
Attachment #404393 - Attachment is obsolete: true
Depends on: 579122
Blocks: 516320
Depends on: 521497
Attached patch patch v4 (obsolete) — Splinter Review
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!
Attachment #457389 - Attachment is obsolete: true
Sweet!
Blocks: 563088
This blocks bug 563088 (turning discarding back on), which is a blocker. Requesting blocking.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
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.
Attachment #459196 - Flags: review?(roc)
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
Attachment #459196 - Flags: review?(roc) → review+
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?
Attached patch patch v5 (obsolete) — Splinter Review
Addressed roc's review comments (still need to look into bz's).
This blocks beta 5, the feature freeze.
blocking2.0: betaN+ → beta5+
Attached patch patch v6 (obsolete) — Splinter Review
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.
Attachment #462609 - Attachment is obsolete: true
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.
Attached patch css-story - part 2 - v1 (obsolete) — Splinter Review
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().
Attachment #463599 - Flags: review?(dbaron)
Attachment #463600 - Flags: review?(dbaron)
Attachment #463601 - Flags: review?(dbaron)
(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.
Attachment #462952 - Flags: superreview?(dbaron)
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
Attachment #463599 - Flags: review?(dbaron) → review+
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.
Attachment #463600 - Flags: review?(dbaron) → review+
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
Attachment #463601 - Flags: review?(dbaron) → review+
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
Attachment #462952 - Flags: superreview?(dbaron) → superreview+
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.
Attachment #459196 - Attachment is obsolete: true
Attachment #462952 - Attachment is obsolete: true
Attachment #459196 - Attachment is obsolete: false
Attachment #462952 - Attachment is obsolete: false
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.
Attachment #463600 - Attachment is obsolete: true
Attachment #462952 - Attachment is obsolete: true
Attachment #459196 - Attachment is obsolete: true
Attachment #466538 - Attachment description: patch v7 → part 1 - v7 - Add image tracker to nsDocument and register content images with it
Attachment #466538 - Flags: superreview+
Attachment #466538 - Flags: review+
Attachment #463599 - Attachment description: css-story - part 1 - v1 → part 2 - v1 - Make ImageRenderer take a pointer to nsStyleImage rather than making a copy
Attachment #466540 - Attachment description: css-story - part 2 - v2 - call nsDocument::AddImage and RemoveImage on images stored in nsStyleBackground → part 3 - v2 - call nsDocument::AddImage and RemoveImage on images stored in nsStyleBackground
Attachment #466540 - Flags: review+
Attachment #463601 - Attachment description: css-story - part 3 - v1 → part 4 - v1 - call nsDocument::AddImage and RemoveImage on images stored in nsStyleContentData
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.
Attachment #466548 - Attachment description: css-story - v1 - lock images in nsStyleList::SetListStyleImage → part 6 - v1 - lock images in nsStyleList::SetListStyleImage
Attachment #466547 - Flags: review?(dbaron)
Attachment #466548 - Flags: review?(dbaron)
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.
Attachment #466547 - Flags: review?(dbaron) → review-
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.
Attachment #466548 - Flags: review?(dbaron) → review-
(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.
Attachment #466716 - Flags: review?(dbaron)
Attachment #466717 - Flags: review?(dbaron)
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
Attachment #466716 - Flags: review?(dbaron) → review+
Comment on attachment 466717 [details] [diff] [review]
part 6 - v2 - lock images in nsStyleList::SetListStyleImage

r=dbaron
Attachment #466717 - Flags: review?(dbaron) → review+
Attachment #466715 - Flags: review+
Attachment #466714 - Flags: review+
Attachment #466712 - Flags: review+
Attachment #466711 - Flags: superreview+
Attachment #466711 - Flags: review+
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
This appears to be causing a crash: bug 588681
Depends on: 588681
Depends on: 589822
No longer depends on: 589822
Depends on: 591560
You need to log in before you can comment on or make changes to this bug.