Closed Bug 245727 Opened 18 years ago Closed 3 years ago

PSM could use nsIStreamLoaderObserver

Categories

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

Other Branch
x86
Linux
enhancement

Tracking

()

RESOLVED WONTFIX

People

(Reporter: Biesinger, Unassigned)

References

()

Details

(Keywords: memory-footprint, Whiteboard: [good first bug][psm-cleanup])

Attachments

(1 file, 6 obsolete files)

PSMContentDownloader (see URL) looks like it could use nsIStreamLoader; that
would avoid having essentially the same code in two places

hmm, would this be a good first bug?
It's a hard first bug, but a reasonable one, in my opinion...
Whiteboard: [good first bug]
Would attachment 150131 [details] [diff] [review] be of any use to potential hackers?
Yeah that shows basically how to use streamloader...
I've had a go at this (based on neil's patch) and have a working build. Question
is, how do I test it?
Attached patch patch v0 (obsolete) — Splinter Review
here's the patch
I guess you have to browser to a "secure" site using a self-signed certificate.
Cool! Tested it on localhost with a self-signed certificate, got a certificate
popup warning and then I'm thru. 

Looking for r=?
So were you able to accept the certificate permanently to survive a restart?

Also, I am somewhat guessing as to the purpose of this code; you should use
debugging techniques (breakpoints / printfs) to verify that your code is used.
Ok, I've tested accepting certificates per session/permanently and used the
Tools | Options | Advance to import/delete/view certificates, but could not get
the code path to execute. I tried both setting breakpoints and PR_LOG-ging and
got no joy.. :(
try installing a root certificate, maybe - https://trust.web.de/root.sql for
example (the "Zertifikat installieren" links near the bottom)
Yeah, it looks like installation of root certs (by clicking on links to them)
and updates of CRL (whatever that is) are all this is used for.
Yay and nay.

Installing a root certificate does indeed invoke the PSM code, however I've now
run into a new problem. The current code uses a nsIURIContentListener to peek at
the content before loading, which expects a nsIStreamListener to be returned.

Since this has been changed, I need a different way to get the nsIStreamListener
object. Can it be obtained from nsIStreamLoaderObserver somehow? Or converted to it?

Alternatively, I can convert PSMContentListener to use nsIContentHandler (like
the addressbook), but then I don't know how to register the handler. My guess is
that I need to change nsNSSModule.cpp, but I am not sure how to do this.
urg. OK. can we make nsIStreamLoader be an nsIStreamListener and have users of
it open the channel itself as needed? (similar to nsIDownloader)
That would also simplify the streamloader code a bit (since it does not need to
handle AsyncOpen failure, which currently makes it post an event)

if we do that, nsIStreamLoader should inherit from nsIStreamListener.
> urg. OK. can we make nsIStreamLoader be an nsIStreamListener and have users of
> it open the channel itself as needed? (similar to nsIDownloader)

Yes, that is something I've considered doing.  (Feel free to file a bug to
propose that change?)  Perhaps another option for this bug might be to use
nsISimpleStreamListener?
Attached patch patch v1 (obsolete) — Splinter Review
Decided to go with bi's suggestion to change nsIStreamLoader to inherit from
nsIStreamListener. After much reading and experimenting, I finally got a
working patch.

The PSM code now works perfectly. The only remaining issue is how to handle the
openning of channels now that it has been removed from nsIStreamLoader. I've
added AsyncOpen calls to the relevant places, but am not sure how to deal with
failed AsyncOpens.
Attachment #172551 - Attachment is obsolete: true
I don't think you should be including patches to the networking library in this
bug.  File a new bug for those and make this bug be dependent on that one.  Then
come back and write a patch to use the new API.  Also, I think that
nsIStreamLoader and nsIUnicharStreamLoader should have parallel APIs, so
whatever change is made to one should be made to the other, which might bloat
your patch a bit.
Ok. Bug 281153 created.
Depends on: 281153
Assigning bug to Son Le, since he is currently working on it.
Assignee: kaie → son.le0
Attached patch patch v2 (obsolete) — Splinter Review
Patch to make PSMContentDownloader inherit from nsIStreamLoaderObserver.
Pending checkin to bug 281153.
Attachment #173362 - Attachment is obsolete: true
Product: PSM → Core
Attached patch patch v3.0 - basic changes (obsolete) — Splinter Review
Basic changes to make PSM use IStreamLoaderObserver.
Attachment #175685 - Attachment is obsolete: true
Uber patch to make PSM use IStreamLoaderObserver as well as OOM checks, and some code cleanup.

Tested CRL import using the "Browser Import" at http://www.grid-support.ac.uk/content/view/182/98 and updates using Options|Advanced|Revocation Lists|Update.

Made sure that certificates survived firefox browser sessions and PSM was properly destroyed. Didn't test the silent update functionality.

All seems to work correctly.
Attachment #245447 - Flags: review?(kengert)
Thanks for your patch.
In addition to the "real" change that is requested in this bug, you are doing a lot of additional cleanup.
I am ok with this in general and appreciate it, but I do have some comments.

---
Please undo your cleanup around entropy collector allocation.

With your code, if allocation of ec fails, we will no longer get the assertion. We'll only get it if ec succeeded, but bec fails.

We should get the assertion if ec fails or if both fail.

---
Please note the purpose of nsPSMUITracker and that it triggers some actions in its constructor and destructor. We should limit the lifetime of this object as much as possible to the real UI execution.

You removed the {} around this object, we should add it back. Also, the call to NSS function PK11_GetTokenName should be executed outside of this scope.

    PRBool canceled;
    NS_ConvertUTF8toUTF16 tokenName(PK11_GetTokenName(slot));

    {
      nsPSMUITracker tracker;
      if (tracker.isUIForbidden()) {
        rv = NS_ERROR_NOT_AVAILABLE;
      }
      else {
        rv = dialogs->SetPassword(ctx, tokenName.get(), &canceled);
      }
    }

---
In PSMContentDownloader::OnStreamComplete, is there a chance that aLoader will be null? I don't know. If it's possible, we should use NS_ENSURE_ARG.


If you address these requests, you'll get my r+
Attachment #245447 - Flags: review?(kengert) → review-
Attached patch patch 3.2 (obsolete) — Splinter Review
New patch addressing review comments.

> In PSMContentDownloader::OnStreamComplete, is there a chance that aLoader will
> be null? I don't know. If it's possible, we should use NS_ENSURE_ARG.

No. The aLoader will actually call this OnStreamComplete function.
Attachment #245446 - Attachment is obsolete: true
Attachment #245447 - Attachment is obsolete: true
Attachment #250455 - Flags: review?(kengert)
QA Contact: ui
It looks like we got close to a fix here, but the latest patch has been sitting unreviewed for two year.  Son, are you still interested in working on this bug?  Otherwise I might grab it if we still feel it's worth it (it's still listed as one of our top "first bugs" for newbies to try fixing).
Jason, feel free to grab it.
OK, I've assigned this to myself.  Son, thanks for all the work you've done on this bug already--I'll try to finally get your code into the tree!  (I bet you didn't expect it to take 5 years for your code to make it in when you started working on this!)
Assignee: son.le0 → jduell
Status: NEW → ASSIGNED
Priority: -- → P4
Is this change going to make it into the final build anytime soon? If more work needs to be done on this let me take a look.
Someone probably needs to prod Kai with a sharp stick over e-mail to review this.
Attached patch Patch v4Splinter Review
I've merged the patch to the current trunk.
Attachment #250455 - Attachment is obsolete: true
Attachment #451606 - Flags: review?(kaie)
Attachment #250455 - Flags: review?(kaie)
Comment on attachment 451606 [details] [diff] [review]
Patch v4

Wonderful patch, great cleanup, thank a lot for working on it.

r=kaie
Attachment #451606 - Flags: review?(kaie) → review+
Is this checkin-needed material? Or (more likely) does this need rebasing again?
I'm going through old "good first bugs" that have been inactive for a while. 
It seems that something is blocking this; As for comment 31 is it a checkin-needed problem?

Kai can you do something to unlock this?
Flags: needinfo?(kaie)
Yes, someone should have reminded me three years ago that checkin is needed... By now, the underlying code has changed a lot, portions have been removed. Someone must merge the old patch to the most recent code, prior to proceeding with this.
Flags: needinfo?(kaie)
Component: Security: UI → Security: PSM
Priority: P4 → P3
Whiteboard: [good first bug] → [good first bug][psm-cleanup]
Assignee: jduell.mcbugs → nobody
Status: ASSIGNED → NEW

Hi, I would like to work on the bug. Is this active?

I think given how the architecture has changed due to process separation, it doesn't make sense to do this any longer.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.