Closed Bug 1339891 Opened 7 years ago Closed 7 years ago

Presshell flushes should short-circuit immediately if the shell doesn't need that flush type

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(2 files, 1 obsolete file)

Now that we've done bug 1334735, we should be able to do this.
Attachment #8837728 - Attachment is obsolete: true
Attachment #8837728 - Flags: review?(cam)
Comment on attachment 8837734 [details] [diff] [review]
Make FlushPendingNotifications on a presshell quickly no-op if there is nothing to flush

>   if (nsIPresShell* shell = GetShell()) {
>-    if (shell->NeedFlush(aType)) {
>-      nsCOMPtr<nsIPresShell> presShell = shell;
>-      presShell->FlushPendingNotifications(aType);
>-    }
>+    shell->FlushPendingNotifications(aType);

This doesn't seem right; callers of FlushPendingNotifications are still
required to hold a strong ref on the shell that outlives that call,
as far as I know.
That depends.  A bunch of them don't, and FlushPendingNotifications itself holds a kungfudeathgrip precisely to deal with it.  I _could_ make the public APIs here take strong refs internally after doing the NeedsFlush() check, or I could make document hold one either with the manual check or just unconditionally.  But it's all noise.

Callers who plan to _use_ the presshell after flushing need to hold a strong ref, of course.  But this caller doesn't do that.
> A bunch of them don't ...

Just because we have consumers that violates the required pre-condition, doesn't excuse
adding one more.  We should either continue to adhere to the caller-holds-strong-ref
requirement, or make FlushPendingNotifications not depend on that in any situation and
then drop that requirement.

> Callers who plan to _use_ the presshell after flushing need to hold a strong ref, of course.

Right, that's not what I'm talking about here.
I think we should drop the precondition, personally.  Especially since it's not documented, so consumers have no way to know it _is_ a precondition.

> or make FlushPendingNotifications not depend on that in any situation and
> then drop that requirement

I think that's where we are right now: FlushPendingNotifications doesn't depend on the caller holding a strong ref, and the requirement is not documented at all.

I'd be happy to add some comments about how the call can destroy the presshell and if you want to keep using it you better hold a strong ref to it.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #6)
> I think we should drop the precondition, personally.

Works for me.  In that case, while we're here, could you remove this comment please:
http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/layout/base/PresShell.cpp#4146-4147

And perhaps move the "nsCOMPtr<nsIPresShell> kungFuDeathGrip;" line to be
the very first line in this method, to avoid someone adding an AutoRestore(member)
before it in the future?

> I'd be happy to add some comments about how the call can destroy the
> presshell and if you want to keep using it you better hold a strong ref to
> it.

Doesn't hurt to raise the awareness I guess.
*to avoid an UAF if someone adds an AutoRestore...
> And perhaps move the "nsCOMPtr<nsIPresShell> kungFuDeathGrip;" line

What I'm trying to recall is whether there's a reason for the ordering there, in particular to make sure the viewmanager outlives the presshell.  See also comments in PresShell::ResizeReflowIgnoreOverride.  I wish I remembered why I added those back in bug 396587...

Of course if the _caller_ were holding the strong ref to the presshell you'd get presshell going away second anyway.
I think the ViewManager is generally expected to outlive the shell, but I think
it works fine as long as PresShell::Destroy was called properly before it's deleted.
We used to have some bugs in that area though; enough that we added a gratuitous
Destroy() call in the destructor:
http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/layout/base/PresShell.cpp#899-901

So if that happens then this will be a use-after-free if the VievManager is gone:
http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/layout/base/PresShell.cpp#1320-1324

It's well over a decade since I last saw that assertion in the dtor though,
so I don't think the order matters nowadays.  We should probably just
remove that call and MOZ_RELEASE_ASSERT there instead.
> So if that happens then this will be a use-after-free if the VievManager is gone:

Ah, that was the story.  Thank you for looking it up!  Looks like the Destroy() in the destructor dates back to bug 80203 and bug 89626 or so...

> We should probably just remove that call and MOZ_RELEASE_ASSERT there instead.

OK, but I'd rather not scope-creep this bug to that.
Daniel, see comment 10 for that thing with ordering we were talking about.  Apparently this is not a problem after all...
Attachment #8838112 - Flags: review?(mats) → review+
Attachment #8837734 - Flags: review?(cam) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32242abfde5d
part 1.  Make the invariants around nsIPresShell::FlushPendingNotifications clearer.  r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d588184435
part 2.  Make FlushPendingNotifications on a presshell quickly no-op if there is nothing to flush.  r=heycam
https://hg.mozilla.org/mozilla-central/rev/32242abfde5d
https://hg.mozilla.org/mozilla-central/rev/44d588184435
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.