293534 - reloading with bfcache will crash Camino (nsDocShell::RestorePresentation(nsISHEntry*, int, int*) + 0x72c)

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Torben]

Camino 2005050908 (v0.8+)

(Found when trying to reproduce bug 245026)

With bfcache enabled (browser.sessionhistory.max_viewers, 5) Camino will crash
when you try to reload the url. No crash with bfcache turned off. Curiously
Firefox (same build date) does _not_ crash with bfcache turned on.
Might, or might not, be related to bug 292969 and/or bug 292970.

Steps to reproduce:
1. Go to http://www.dioceseofstasaph.org.uk/
2. Click on a link to "Diocesesan organisations" in the left column
3. Click the reload button

Expected result:
Reload page

Actual result:
Crash

Trackback ID TB5704257M, crashlog coming up

Comment 1

[Torben]

Attachment: Crash log

Comment 2

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050510] (nightly) (W98SE)

Confirmation: MozillaAS doesn't crash at step 3 either.
(bug I can get bug 293698 going Back at a fourth step)

Comment 3

[Brian Ryner (not reading)]

Attachment: patch

The problem here happens as follows:

- The site contains a <frameset> with a single <frame>
- When you click a link in the frame, the session history tree is cloned; the
clone does not have any cached presentation.  The original history tree (a
parent entry and one child) has cached content viewers for both the parent and
the child.
- When you reload, the <frame> and its corresponding docshell are destroyed. 
The child history entry's viewer now has a dangling container pointer.
- Go to restore the child history entry -> crash

There are basically 3 things we can do:
- hold a strong reference to the docshell; swap it back into the docshell tree
when restoring the <frame>
- hold a weak reference; bail on restoring the presentation if the docshell has
gone away
- hold a weak reference; try to restore the presentation into the _new_
docshell if the docshell has gone away

I've opted to go for the second one.  This might be worth revisiting if it
affects a significant number of sites / use cases, but the other options seem
pretty scary to me.

I originally wanted to do this just by making Open() fail, but it turns out
that we don't unhook the old content viewer correctly when that happens,
leading to crashes.  I just added an up-front check that the container of the
stored content viewer is in fact the docshell we're restoring it to, which is a
good invariant even if having Open() fail worked as expected.

Comment 4

[Brian Ryner (not reading)]

In addition to this:

+    if (container !=
nsCOMPtr<nsISupports>(do_QueryInterface(NS_STATIC_CAST(nsIDocShell*, this)))) {
+#ifdef DEBUG_PAGE_CACHE
+        printf("No valid container, not restoring presentation\n");
+#endif
+        return NS_OK;
+    }

I wonder if it would make sense to clear out the whole presentation on the
history entry...

Comment 5

[Torben]

FWIW, my Camino won't build with the above patch (attachment (id=183352))

nsDocShell.cpp: In member function `nsresult 
   nsDocShell::RestorePresentation(nsISHEntry*, int, PRBool*)':
nsDocShell.cpp:5180: error: `uri' undeclared (first use this function)
nsDocShell.cpp:5180: error: (Each undeclared identifier is reported only once 
   for each function it appears in.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

>Index: docshell/base/nsDocShell.cpp

> #ifdef DEBUG_PAGE_CACHE
>+    nsCOMPtr<nsIURI> uri;
>+    aSHEntry->GetURI(getter_AddRefs(uri));

|uri| is passed to SetCurrentURI at the end of this method, so we can't move
getting it into this ifdef....

>@@ -4970,6 +4970,21 @@ nsDocShell::RestorePresentation(nsISHEnt
>+    if (container != nsCOMPtr<nsISupports>(do_QueryInterface(NS_STATIC_CAST(nsIDocShell*, this)))) {

I'd prefer:

  if (!::SameCOMIdentity(container, GetAsSupports(this))) {

here.

And yes, clearing out the presentation on the history entry when we hit this
case makes a lot of sense to me.

>Index: layout/base/nsDocumentViewer.cpp
>@@ -610,10 +611,7 @@ DocumentViewerImpl::GetContainer(nsISupp

>+   return CallQueryReferent(mContainer.get(), aResult);

Won't that crash if mContainer is null (eg a container was never set)?	Is that
something we should worry about?  If so, may be worth using do_QueryReferent
and then swap() here.

>@@ -1466,13 +1471,19 @@ DocumentViewerImpl::SetDOMDocument(nsIDO
>+  nsCOMPtr<nsIScriptGlobalObject> global;
>+  if (requestor) {
>+    CallGetInterface(requestor.get(),
>+                     NS_STATIC_CAST(nsIScriptGlobalObject**,
>+                                    getter_AddRefs(global)));
>+  }

Is this not using do_GetInterface(requestor); just to avoid an extra QI?  If
so, I'd go with the extra QI and clearer code...  but either way.

>@@ -4018,7 +4032,14 @@ DocumentViewerImpl::OnDonePrinting() 
>+      nsCOMPtr<nsIDOMWindowInternal> win;
>+      if (requestor) {
>+        CallGetInterface(requestor.get(),
>+                         NS_STATIC_CAST(nsIDOMWindowInternal**,
>+                                        getter_AddRefs(win)));
>+      }

Same here.

r+sr=bzbarsky with the nits picked.

Comment 7

[Torben]

Attachment: Crash log 2

Camino will still crash with this patch (after moving the definition of uri
outside the #ifdef or #ifdef'ing the SetCurrentURI) if after reloading the page
I click on the link to "Diocesesan organisations" again. The crash now happens
in nsChildView::Destroy().

Comment 8

[Brian Ryner (not reading)]

(In reply to comment #6)
> >@@ -1466,13 +1471,19 @@ DocumentViewerImpl::SetDOMDocument(nsIDO
> >+  nsCOMPtr<nsIScriptGlobalObject> global;
> >+  if (requestor) {
> >+    CallGetInterface(requestor.get(),
> >+                     NS_STATIC_CAST(nsIScriptGlobalObject**,
> >+                                    getter_AddRefs(global)));
> >+  }
> 
> Is this not using do_GetInterface(requestor); just to avoid an extra QI?  If
> so, I'd go with the extra QI and clearer code...  but either way.
> 

Yep, trying to avoid the extra QI when we already have an nsIInterfaceRequestor.
 I wonder if it would make more sense to add an overloaded do_GetInterface()
implementation for nsIInterfaceRequestor*.

Comment 9

[Brian Ryner (not reading)]

I'll do a Camino build this afternoon and try to figure out what's going on.

(In reply to comment #7)
> Created an attachment (id=183434) [edit]
> Crash log 2
> 
> Camino will still crash with this patch (after moving the definition of uri
> outside the #ifdef or #ifdef'ing the SetCurrentURI) if after reloading the page
> I click on the link to "Diocesesan organisations" again. The crash now happens
> in nsChildView::Destroy().

Comment 10

[Brian Ryner (not reading)]

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

I'm not seeing Camino crash using this patch.  I did address Boris's comment
regarding possible crash with CallQueryReferent, so maybe that's the reason. 
Either way, I'd like to get this in.

Comment 11

[Brendan Eich [:brendan]]

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

a=me with comments addressed.

/be

Comment 12

[Brian Ryner (not reading)]

checked in

Comment 13

[Hermann Schwab]

crashed Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050514

I know this build doesn´t contain the patch, and I couldn´t reproduce the crash.
I´ve seen some sort of runtime error, didn´t care about text or number, as I
wanted to reproduce after enabling talkback by deleting compreg.dat.

Comment 14

[Torben]

No more crashes for me either with the checked in patch.