81796 - Enable SecureBrowserUI for embedding apps

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Conrad Carlen (not reading bugmail)]

Currently, an embedding app has to do some work in order to create an instance
of nsISecureBrowserUI for each top level window. Without doing so, they will not
receive any security notifications through nsIWebProgressListener. This is, to
say the least, not obvious. I suggest we create the nsISecureBrowserUI instance
in the webBrowser lib in order to hide step this from embeddors.

Comment 1

[Chak Nanga]

Attachment: Patch to create nsISecureBrowserUI during the creation of the webbrowser object

Comment 2

[Chak Nanga]

Conrad had some corerns about exposing nsISecureBrowserUI as an attribute of 
nsIWebBrowser.

With the above patch this is what embedding apps will have to do do display 
security info. such as doing view certificate:

1. Get nsISecureBrowserUI from nsIWebBrowser (which they already have)
2. QI nsISecureBrowserUI for nsISSLStatusProvider.
3. Get the nsISSLStatus attribute from nsISSLStatusProvider.
4. Get cert info. etc. from nsISSLStatus.

I've got this scheme working in the mfcembed sample which embeddors can use as a 
starting point.

Please let me know if should be giving them some other interface than 
nsISecureBrowserUI. May be we give them access to an nsISSLStatus?

Comment 3

[Chak Nanga]

Attachment: Patch to get nsISecureBrowserUI via a GetInterface rather tahn as an attribute.

Comment 4

[Chak Nanga]

Hi Terry:

Could you please let me know if giving embeddors access to nsISecureBrowserUI is 
the way to go.

If yes, i need an r= and sr=
If no, please let me know the PSM interface we should be giving out

Thanks

Comment 5

[Adam Lock]

Is it possible to create the secure UI on demand so we don't have the bloat if 
it's never used?

Also, can you update your mfcEmbed example to use the GetInterface version of 
this patch?

Comment 6

[Conrad Carlen (not reading bugmail)]

> Is it possible to create the secure UI on demand so we don't have the bloat if 
it's never used?


I don't think so. When the component is created, it installs listeners which
have to be there initially.

Comment 7

[Terry Hayes]

Sorry to be so late with my comments:

Providing access to nsISecureBrowserUI isn't the most useful thing, since the
methods there are directly useful, and the client will need to QI to do
anything. It might be better to provide access to the nsISSLStatusProvider
interface, or directly to the nsISSLStatus attribute.

However, providing a direct attribute for nsISSLStatusProvider (say) would make
you depend on PSM interfaces in the web browser interface.  I think the
implementation already depends on PSM, so that may not be an issue.

If you don't mind adding the dependency, then I would add the
nsISSLStatusProvider to the web browser as an attribute.  If you don't like the
dependancy (in the interface) you could have your implementation implement
nsISSLProvider and allow the client to QI for it.

At some level you need the PSM interfaces to display the data, it's just a
question of whether that is assumed at the very low (browser interface) level.
Otherwise the interfaces you are using are fine. 

Comment 8

[Judson Valeski]

if we expose
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/public/nsISSLStatus.idl#49
either/both of these ifaces, they'll become frozen. Terry, are you prepared for
that? Also, nsIX509Cert gets pulled in, which would inter freeze its entrails as
well.

Comment 9

[Conrad Carlen (not reading bugmail)]

> However, providing a direct attribute for nsISSLStatusProvider (say) would make
you depend on PSM interfaces in the web browser interface.

Using a GetInterface instead of an attribute removes that concern. That was my
objection as well to using an attribute.

Jud's point is good. Should we have another layer of API which would be exosed
to embeddors to insulate us a bit from the deep internals of PSM?

Comment 10

[Terry Hayes]

(adding cc for Javi and David)

It would be a useful exercise for you to define the embedding view of the
security as a new API.  That would tell us what things are needed at that layer.
 If you end up covering most of the PSM intefaces anyway, we should just
standardize those.  However if you don't need certs, or ciphernames, or key
strength values (...) then we can just implement your frozen interfaces using
the underlying PSM ones.

Comment 11

[Judson Valeski]

heh, I was hoping you guys could provide the "what's needed" view :-). I have no
clue, and I suspect most embeddors don't have a clue either, so, you tell us
what you think the top properties should be, and we'll expose them through some
nsISecurePageInfo type iface, and modify that as true requirements surface.

Comment 12

[Chak Nanga]

I think we should not be coming up with a new interface based on our assumptions 
on what embeddors may want or not. Some may be interested in the cert 
functionality while others may not care for it.

If we came up with a new intermediate interface, i'm pretty sure down the road 
there'll be someone who wants the missing functionality in the new interface(and 
those that are available in the PSM interfaces)

Comment 13

[Chak Nanga]

Since it might be a while before we decide on what's the appropriate 
security info interface to expose to embeddors jud mentioned that i should at 
least go ahead and hook into the OnSecurityChange() notification for the lock 
icon updates.

I'll submit a patch with those changes shortly and would appreciate a r= and 
sr=.

Comment 14

[Chak Nanga]

Attachment: Patch with just the changes to hook into OnSecurityChange()

Comment 15

[Conrad Carlen (not reading bugmail)]

Now the patch fits the summary of the bug. I'd file another one on allowing
embeddors to get page security info.

One little nit:
+   mBackgroundColor(0), mSecurityUI(nsnull)
You don't need that. The nsCOMPtr constructor initializes itself.

r=ccarlen

Comment 16

[Chak Nanga]

*** Bug 63323 has been marked as a duplicate of this bug. ***

Comment 17

[Christopher Blizzard (:blizzard)]

With conrad's comment fixed sr=blizzard

Comment 18

[Chak Nanga]

Attachment: Updated patch based on Conrad's comments i.e. no need to init mSecurityUI

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

a=dbaron for 0.9.1

Comment 20

[Chak Nanga]

Fix checked in

Comment 21

[Michael Dunn]

Clean up verification of dated code change bus