534226 - Remove support for multiple presshells

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug]]

.

Comment 1

[Olli Pettay [:smaug]]

I can do this, after bug 500882 is fixed.

Comment 2

[Olli Pettay [:smaug]]

Attachment: patch

Tried to keep the changes pretty minimal (even in nsDocument), and made presshell a raw ref only if I was sure that nothing bad could happen.

Didn't rename GetPrimaryShell to GetShell to keep the patch size reasonable.

Comment 3

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 420318 [details] [diff] [review]
patch

>+++ b/content/base/public/nsIDocument.h
>+  void DeleteShell(nsIPresShell* aShell)

Did we just keep the argument so we can assert about it?  If so, do we want to nix that argument altogether in a followup and move the assert into the presshell code or something?

>+  nsIPresShell* GetPrimaryShell() const

Followup bug on whatever renaming we want here, right?

>+++ b/content/base/src/nsContentSink.cpp
> nsContentSink::ScrollToRef()
>-  if (mRef.IsEmpty()) {
>+  if (mRef.IsEmpty() || !mDocument) {


Why is this change needed?

>+  // Make sure we don't call InitialReflow() for a shell that has
>+  // already called it. This can happen when the layout frame for
>+  // an iframe is constructed *between* the Embed() call for the
>+  // docshell in the iframe, and the content sink's call to OpenBody().
>+  // (Bug 153815)

I _think_ that now that we're guaranteed to only have one shell at a time this might in fact be impossible.  Maybe a followup bug to investigate and simplify as needed if we care?

We should probably audit the iterator callsites you're changing and see which of them actually need a strong presshell ref...  The iterator forces all consumers to have one.

>+++ b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>@@ -87,33 +86,29 @@ nsSVGMutationObserver::AttributeChanged(

Good catch here. Oops.  ;)

r=bzbarsky.

Comment 4

[Olli Pettay [:smaug]]

(In reply to comment #3)
> Did we just keep the argument so we can assert about it?
Yes.
>  If so, do we want to
> nix that argument altogether in a followup and move the assert into the
> presshell code or something?
Or I just move the assertion to presshell in this bug. Simple thing.

> Followup bug on whatever renaming we want here, right?
Ok

> Why is this change needed?
Ah, right, it isn't needed after all.
I was just trying to be over-careful with null checks.

> I _think_ that now that we're guaranteed to only have one shell at a time this
> might in fact be impossible.  Maybe a followup bug to investigate and simplify
> as needed if we care?
Ok

> 
> We should probably audit the iterator callsites you're changing and see which
> of them actually need a strong presshell ref...  The iterator forces all
> consumers to have one.
Yeah. I didn't want to do that in this patch.

Comment 5

[Olli Pettay [:smaug]]

Attachment: +comment

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 420393 [details] [diff] [review]
+comment

We need to rename GetPrimaryShell to GetShell, but that doesn't have to be this patch.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> > We should probably audit the iterator callsites you're changing and see which
> > of them actually need a strong presshell ref...  The iterator forces all
> > consumers to have one.
> Yeah. I didn't want to do that in this patch.

We don't have to do the audit in this patch, but it would make sense to do it in this bug IMHO, or otherwise soon, since otherwise we'll lose track of which sites need to be audited.

Comment 8

[Olli Pettay [:smaug]]

http://hg.mozilla.org/mozilla-central/rev/69409debef05

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

>    NS_ASSERTION(mDocument->GetPrimaryShell() == this, "Wrong shell?");

Wouldn't that fire any time a presshell for a bfcached document (which returns null for GetPrimaryShell()) is destroyed?

Comment 10

[Olli Pettay [:smaug]]

Seems like SetShellHidden(PR_FALSE) is called just before dropping
contentviewer from bfcache.

Comment 11

[Boris Zbarsky [:bzbarsky, bz on IRC]]

OK.  Let's keep things this way, then.