1493655 - nsISecureBrowserUI::Init should take a docshell instead of a content window

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[:Gijs (he/him)]

The only thing the nsSecureBrowserUIImpl constructor does with the window is get the docshell.

Instead, we could just pass the docshell directly. This would reduce code size and avoid constructing an about:blank content window in the docshell when constructing the security UI / info object.

This seems a pretty simple change so, assuming that doesn't turn out to be an illusion, I'd like to do that in this bug, to fix the immediate problem I'm seeing where the securityUI initialization forces initialization of the content window - plus tidying up some code that currently gets a content window reference for no reason other than to construct this object.

Longer-term though, I wonder why consumers are responsible for the construction of this object. I guess there's a performance impact when constructing it for docshells that don't need it (ie have no security UI). On the other hand, perhaps we could rework the implementation so it can be fetched "just in time". That is, make docshell construct one if someone requests it and it doesn't exist yet, and make the securityInfo object populate its mState member if it's 0/uninitialized, and someone asks it for the security state, using the docshell's document channel (after which it'll then be updated the usual way).

Comment 1

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #0)
> Longer-term though, I wonder why consumers are responsible for the
> construction of this object. I guess there's a performance impact when
> constructing it for docshells that don't need it (ie have no security UI).
> On the other hand, perhaps we could rework the implementation so it can be
> fetched "just in time". That is, make docshell construct one if someone
> requests it and it doesn't exist yet, and make the securityInfo object
> populate its mState member if it's 0/uninitialized, and someone asks it for
> the security state, using the docshell's document channel (after which it'll
> then be updated the usual way).

Hm. I looked at this a bit, but I don't see a good way of making this lazy today - it looks and feels as if consumers explicitly construct this today because they want the onSecurityChange() notifications, and they don't otherwise update anything, so if we push responsibility into docshell we'd have to create it eagerly, or we'd miss the notifications. That's definitely possible, but seems like a bit of work that we don't necessarily need to do here, also because we'd want to avoid doing it for chrome type docshells etc. etc.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1493655 - make nsISecureBrowserUI initialize from a docshell instead of a window, r?keeler,nika

This also removes the (afaict, unused) stub implementation from TabParent. The netwerk header
inclusions were necessary because those files included TabParent.h and through it,
nsISecureBrowserUI, but now TabParent.h no longer does that.

Comment 3

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

Comment on attachment 9011917 [details]
Bug 1493655 - make nsISecureBrowserUI initialize from a docshell instead of a window, r?keeler,nika

Dana Keeler [:keeler] (she/her) (use needinfo) has approved the revision.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9011917 [details]
Bug 1493655 - make nsISecureBrowserUI initialize from a docshell instead of a window, r?keeler,nika

:Nika Layzell has approved the revision.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d86f6268d191
make nsISecureBrowserUI initialize from a docshell instead of a window, r=keeler,nika

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/d86f6268d191