372107 - Make nsDocLoader/nsDocShell/nsWebShell participate in cycle-collection

Status: RESOLVED DUPLICATE of bug 1401379

(Core :: DOM: Navigation, defect, P1)

Description

[Peter Van der Beken [:peterv]]

We want to make nsDocLoader/nsDocShell/nsWebShell participate in cycle-collection, but the cycle-collector is main-thread only. Can we make these objects main-thread only too?

Comment 1

[Boris Zbarsky [:bzbarsky]]

As a start, look at where a current trunk build hits thread-safety assertions in PSM due to its getting the (not thread safe) security UI from the (threadsafe) docshell on a background thread:

#2  0x04cdb78f in nsSecureBrowserUIImpl::AddRef (this=0x9af5990)
    at ../../../../../mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp:167
#3  0x01e112fe in ns_if_addref<nsISecureBrowserUI*> (expr=0x9af5990)
    at ../../dist/include/xpcom/nsISupportsUtils.h:114
#4  0x01def254 in nsDocShell::GetSecurityUI (this=0xb1c6de98, aSecurityUI=0xb7d88ed0)
    at ../../../mozilla/docshell/base/nsDocShell.cpp:1479
#5  0x020fbacf in nsNSSSocketInfo::SetNotificationCallbacks (this=0xb1dd7600, 
    aCallbacks=0xb1e111d4)
    at ../../../../../mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:336
#6  0x081f9565 in nsSocketTransport::BuildSocket (this=0xb465a390, fd=@0xb7d8918c, 
    proxyTransparent=@0xb7d89188, usingSSL=@0xb7d89184)
    at ../../../../mozilla/netwerk/base/src/nsSocketTransport2.cpp:1030

Is touching the docshell here at all valid from the POV of the nsIChannel::notificationCallbacks API?  If so, then things are a little unfortunate...

Although, there is one ray of light: the notification callbacks are not the docshell itself, but a shim object that holds a weak ref to the docshell.  We could make this object sync-proxy stuff to the main thread, I suppose.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Though we could still have situations where a docshell is _owned_ by a different thread, indirectly, via a channel's callbacks of some sort.  But I think channels do all refcounting of that sort on the main thread (or ought to), so that might be good enough for the collector.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Do we want to try for this in 1.9? I could use it for bug 419655...

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Also have to remember that nsFrameLoader tries to traverse nsDocShell, and that does nothing...

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #3)
> I could use it for bug 419655...

Fixed that bug without this one, so no longer vital for 1.9 as far as I know... Would be good to fix nonetheless.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This patch applies on top of attachment 311700 [details] [diff] [review] in bug 243591. I had to get this fixed to narrow down bug 423723. Anyway, as you'd expect it adds the docshell and uriloader classes to cycle collection and fixes up some other untraversed edges as well.

Comment 7

[Damon Sicore (:damons)]

marking wanted1.9.1? to get this in the triage queue.  If this needs to be blocking1.9.1?, please mark it as so.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

This is an updated patch, applies on top of the patch for bug 436379 (not on top of the patch in bug 243591 any longer!).

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 323793 [details] [diff] [review]
Patch, v2

>diff -NprU8 ccdocshell.5c13afdb2fa7/content/xul/templates/src/nsXULContentBuilder.cpp ccdocshell.0935e6eb0ca4/content/xul/templates/src/nsXULContentBuilder.cpp

>+    NS_DECL_ISUPPORTS_INHERITED

>+NS_IMPL_ISUPPORTS_INHERITED0(nsXULContentBuilder, nsXULTemplateBuilder)

Strictly speaking we don't really need these changes. If you keep them you should remove nsXULTemplateBuilder::Traverse and nsXULContentBuilder::Traverse and traverse mSortState here.

>diff -NprU8 ccdocshell.5c13afdb2fa7/content/xul/templates/src/nsXULTemplateBuilder.cpp ccdocshell.0935e6eb0ca4/content/xul/templates/src/nsXULTemplateBuilder.cpp

> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsXULTemplateBuilder)
>   NS_INTERFACE_MAP_ENTRY(nsIXULTemplateBuilder)
>   NS_INTERFACE_MAP_ENTRY(nsIDocumentObserver)
>   NS_INTERFACE_MAP_ENTRY(nsIMutationObserver)
>   NS_INTERFACE_MAP_ENTRY(nsIObserver)
>+  NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)

>         gObserverService->AddObserver(this, DOM_WINDOW_DESTROYED_TOPIC,
>-                                      PR_FALSE);
>+                                      PR_TRUE);

I wonder if we should have this in a separate patch for bug 394514 instead (and remove all the DOM_WINDOW_DESTROYED_TOPIC code).

>diff -NprU8 ccdocshell.5c13afdb2fa7/docshell/base/nsDocShell.cpp ccdocshell.0935e6eb0ca4/docshell/base/nsDocShell.cpp

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsDocShell, nsDocLoader)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mContentViewer)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mScriptGlobal)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mSessionHistory)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mGlobalHistory)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mFind)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mOSHE)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mLSHE)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mSecurityUI)

Some of these don't participate themselves, right? In general I don't like adding stuff that doesn't participate.

>diff -NprU8 ccdocshell.5c13afdb2fa7/dom/src/base/nsGlobalWindow.cpp ccdocshell.0935e6eb0ca4/dom/src/base/nsGlobalWindow.cpp

>+  // Traverse timeouts
>+  for (nsTimeout* timeout = tmp->FirstTimeout();
>+       tmp->IsTimeout(timeout);
>+       timeout = timeout->Next()) {
>+    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "timeout->mWindow");
>+    cb.NoteXPCOMChild(static_cast<nsIScriptGlobalObject*>(timeout->mWindow.get()));
>+    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "timeout->mScriptHandler");
>+    cb.NoteXPCOMChild(timeout->mScriptHandler.get());

So I have a similar patch, but it makes nsTimeout itself participate because it has a refcount. Let me know if you think this is not a problem, or if you want that patch :-).

>diff -NprU8 ccdocshell.5c13afdb2fa7/layout/base/nsDocumentViewer.cpp ccdocshell.0935e6eb0ca4/layout/base/nsDocumentViewer.cpp

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DocumentViewerImpl)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDeviceContext)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDocument)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mViewManager)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mPresContext)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mPresShell)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mSelectionListener)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mFocusListener)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mPreviousViewer)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mSHEntry)
>+#if NS_PRINT_PREVIEW
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mCachedPrintSettings)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mCachedPrintWebProgressListner)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mPrintEngine)
>+#endif // NS_PRINT_PREVIEW

Only add objects that participate themselves (or that we're planning to make participate very soon).

>+#ifdef DEBUG
>+  if (tmp->mInLoadComplete) {
>+    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "kungFuDeathGrip in LoadComplete()");
>+    cb.NoteXPCOMChild(static_cast<nsIContentViewer*>(tmp));

Sooo, this looks a bit dangerous once we start unlinking? kungFuDeathGrip really means we don't want to collect/unlink.

>+#ifdef DEBUG
>+  // The kungFuDeathGrip above will piss off the Cycle Collector and make this

Heh.

>diff -NprU8 ccdocshell.5c13afdb2fa7/layout/base/nsPresShell.cpp ccdocshell.0935e6eb0ca4/layout/base/nsPresShell.cpp

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(PresShell)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR(mDocument)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR(mPresContext)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR(mViewManager)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mLastAnchorScrolledTo)

Maybe need to add mCurrentEventContent/mCurrentEventContentStack?
Maybe need to add mStyleSet->mBindingManager?

>diff -NprU8 ccdocshell.5c13afdb2fa7/layout/tables/nsCellMap.h ccdocshell.0935e6eb0ca4/layout/tables/nsCellMap.h

>-  nsCOMPtr<nsPresContext> mPresContext;
>+  nsPresContext* mPresContext;

Is this safe?

>diff -NprU8 ccdocshell.5c13afdb2fa7/toolkit/components/satchel/src/nsFormFillController.cpp ccdocshell.0935e6eb0ca4/toolkit/components/satchel/src/nsFormFillController.cpp

Ugh, mDocShells should just be nsCOMArray<nsIWeakReference> :-/.

>diff -NprU8 ccdocshell.5c13afdb2fa7/uriloader/base/nsDocLoader.cpp ccdocshell.0935e6eb0ca4/uriloader/base/nsDocLoader.cpp

> NS_INTERFACE_MAP_BEGIN(nsDocLoader)
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIRequestObserver)
>+   NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(nsDocLoader)

Why not

NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsDocLoader)
    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIRequestObserver)

?

>+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsDocLoader)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDocumentRequest)
>+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mLoadGroup)

They don't participate afaik.

>diff -NprU8 ccdocshell.5c13afdb2fa7/uriloader/base/nsURILoader.cpp ccdocshell.0935e6eb0ca4/uriloader/base/nsURILoader.cpp

>-  nsCOMPtr<nsIInterfaceRequestor> m_originalContext;
>+  nsIInterfaceRequestor* m_originalContext;

Is this safe?

Comment 10

[Boris Zbarsky [:bzbarsky]]

That last weak ref (for m_originalContext) is most emphatically NOT safe.

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 323793 [details] [diff] [review]
Patch, v2

r- based at least on comment 10.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 323793 [details] [diff] [review]
Patch, v2

Review of attachment 323793 [details] [diff] [review]:
-----------------------------------------------------------------

::: ccdocshell.5c13afdb2fa7/security/manager/boot/src/nsSecureBrowserUIImpl.cpp
@@ +202,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(nsSecureBrowserUIImpl)
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsSecureBrowserUIImpl)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mToplevelEventSink)

I know nothing about the cycle collector. I would like to understand at least the PSM parts of this patch. In particular, in nsSecureBrowserUIImpl, why is only mToplevelEventSink included in the traversal here? I am guessing it is because it is the only non-weak reference that may possibly have cycles.

But, what about mTransferringRequests? How are we able to determine that no cycles are possible there?

Comment 13

[Andrew McCreight [:mccr8]]

Generally, you only need to tell the cycle collector about ref-counted pointers, because those are the cycles the CC is trying to break.  I'm not familiar with this code, but it looks like the hash table is storing nsIRequest*, which aren't ref-counted.  There are some container classes that do add ref their contents, and thus must be traversed.

Comment 14

[Andrew McCreight [:mccr8]]

Mystor fixed this in bug 1401379. Hooray!