Now that we've done bug 1334735, we should be able to do this.

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=17c3078359f27a81d1b6a60ebd1fe4d9646b642b

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...

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