Make nsDocLoader/nsDocShell/nsWebShell participate in cycle-collection

RESOLVED DUPLICATE of bug 1401379

Status

()

defect
P1
normal
RESOLVED DUPLICATE of bug 1401379
13 years ago
2 years ago

People

(Reporter: peterv, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
mozilla1.9
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
wanted1.9.1 ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

13 years ago
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?
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.
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.
Do we want to try for this in 1.9? I could use it for bug 419655...
Blocks: 419655
Flags: blocking1.9?
Target Milestone: mozilla1.9alpha3 → mozilla1.9
Also have to remember that nsFrameLoader tries to traverse nsDocShell, and that does nothing...
(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.
Flags: blocking1.9? → wanted-next+
Posted patch Patch, v1 (obsolete) — Splinter Review
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.
Assignee: peterv → bent.mozilla
Status: NEW → ASSIGNED
marking wanted1.9.1? to get this in the triage queue.  If this needs to be blocking1.9.1?, please mark it as so.
Flags: wanted1.9.1?
Priority: -- → P1
Posted patch Patch, v2Splinter Review
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!).
Attachment #311702 - Attachment is obsolete: true
Attachment #323793 - Flags: review?
Attachment #323793 - Flags: review? → review?(peterv)
Reporter

Comment 9

11 years ago
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?
That last weak ref (for m_originalContext) is most emphatically NOT safe.
Reporter

Comment 11

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

r- based at least on comment 10.
Attachment #323793 - Flags: review?(peterv) → review-
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?
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.
See Also: → 1401379
Mystor fixed this in bug 1401379. Hooray!
Assignee: bent.mozilla → nobody
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
See Also: 1401379
Duplicate of bug: 1401379
You need to log in before you can comment on or make changes to this bug.