409280 - nsIProtectedAuthThread is not embedding friendly

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P3)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

Bug 119500 introduced the nsIProtectedAuthThread interface:

  46 interface nsIProtectedAuthThread : nsISupports
  47 {
  48   /**
  49    * login - run the thread
  50    *   A user interface implementing this interface needs to
  51    *   call this method as soon as the message to the user is
  52    *   displayed. This will trigger login operation. No user 
  53    *   cancellation is possible during login operation.
  54    */
  55   void login(in nsIDOMWindowInternal dialog);
  56 
  57   /**
  58    * Gets token to be logged in name.
  59    */
  60   wstring getTokenName();
  61 };

As you can see, login() is passed a handle to the dialogue shown to the user. The problems are that this is a nsIDOMWindowInteranal which is not frozen; and also that embedders generally may not have a DOM window for their dialogue (when they use a native dialogue). Since the only purpose of this handle is to call close() on it to disappear the dialogue, couldn't this use something generic, for example nsICancelable, instead ?

(Also, less importantly, why is getTokenName wstring instead of AString) ?

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: proposed patch

I chose to use the same way that bug 223310 used to fix the same issue in nsIKeygenThread, that is to use a nsIObserver implementation.

Unfortunately I don't see any way to test this patch... but it's codewise identical to what nsIKeygenThread does.

I think the implementor should have access to more than just the name of the token, so I also added the nsIPKCS11Slot property.

A question: how hard is it to add an analogue to nsIKeygenThread::UserCancelled, so that the user can cancel the login from the dialogue ?

Comment 2

[Christian Persch (GNOME) (away; not receiving bug mail)]

I think this should block 1.9. This API was added just recently in trunk, and it shouldn't be released in this embedder unfriendly state.

Comment 3

[Mike Schroepfer]

Based on comment 3 moving this to blocking

Comment 4

[Kai Engert (:KaiE:)]

Christian, thanks a lot for your work on this patch!
It seems very reasonable, although I would like to proposal some naming changes.

The existing variables mStatusDialogClosed and mStatusDialogPtr use names that make it obvious they are related to another.

By removing mStatusDialogPtr and switching it to a variable named mObserver it becomes less obvious what this is about.

To keep them in synch I propose:
- rename mStatusDialogClosed to mStatusObserverNotified
- rename mObserver to mStatusObserver

Also, the topic string "login-finished" is very special purpose, I propose we use a more general string. (For example there is a clone of this code used to display a key generation status, and "login-finished" would not make sense in that context.)

I propose to change "login-finished" to "request-close" or "operation-completed".

r=kengert with those changes.

Comment 5

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch

I renamed the member variables as suggested, and used 'operation-completed' as notification topic.

Kai, can you make that r+sr, or should I seek another superreviewer?

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 297247 [details] [diff] [review]
updated patch

Thanks Christian, r=kengert

Comment 7

[Kai Engert (:KaiE:)]

Do you want me to check it in?

Comment 8

[Christian Persch (GNOME) (away; not receiving bug mail)]

Yes, please.

Comment 9

[Reed Loden [:reed]]

Please assign bugs to yourself when you submit patches to fix them. :) If kaie doesn't have time, I can check this in for you.

Comment 10

[Kai Engert (:KaiE:)]

checked in, marking fixed

Comment 11

[Kai Engert (:KaiE:)]

Sounds like this patch introduced a crash, see bug 422947.

Would you be able to have a look? Thanks a lot, if you can.