305660 - Clarify ownership model for nsISecureBrowserUI

Status: RESOLVED WORKSFORME

(Core :: DOM: Navigation, defect)

Description

[Simon Fraser [no longer active]]

Bug 303839 reveals a leak caused by a ref cycle between docShell and its
nsISecureBrowserUI, since nsDocShell retains its nsISecureBrowserUI, and the
nsSecureBrowserUIImpl retains the docShell via mToplevelEventSink.

In addition, nsWebBrowser assumes knowledge of the implementation of
nsISecureBrowserUI, since the block in nsWebBrowser::Create() where we create
and init the nsISecureBrowserUI doesn't retain a reference (it "knows" that
nsSecureBrowserUIImpl gets from the domWindow to the docShell, and calls
docShell->SetSecurityUI()).

All this needs cleaning up. It seems that nsWebBrowser should be the one to call
docShell->SetSecurityUI(), and that nsISecureBrowserUI should be able to clean
up its cycle-causing back-references somehow.

Comment 1

[Serge Gautherie (:sgautherie)]

(Is this related to bug 372107 ?)

Comment 2

[:Gijs (he/him)]

I think this is no longer the case now that Dana reworked this, so we can close as wfm - Dana, is that right?

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I think the ownership is clear at this point (docShell owns nsSecureBrowserUIImpl, nsSecureBrowserUIImpl only holds weak references to the docShell/web progress), but comment 0 is still correct in that the setup is needlessly complicated. Maybe let's just morph this a bit?

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Actually I guess bug 1493655 would address this.