107840 - Need to enable automatic update for crls

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Rangan Sen]

Need to have automatic update capabilities for CRLs. It should be possible to
confogure a crl for automatic update, based on either next update date or a
certain frequency [e.g. every N days]

Comment 1

[Rangan Sen]

Attachment: The download successful window. Clicking OK opens the edit prefs window

Comment 2

[Rangan Sen]

Attachment: Edit prefs Dialog. Clicking OK saves the prefs.

Comment 3

[Rangan Sen]

Attachment: new CRL manager window. clicking advanced opens autoupdate prefs window. Update imports the CRL directly

Comment 4

[Rangan Sen]

Here is the basic overview of how automatic update of crls work :

When a crl is downloaded successfully for the first time, the import successful
dialog pops [attachment 56135 [details]]. Clicking OK opens the auto update prefs window
[attachment 56138 [details]]. Clicking cancel closes the 'download successful' window. 
Note: If the crl already exists, and this is just a update, the 'download
successful' window does not the display the 'Do you want to enable auto update'
question, and clicking OK would close the window. No cancel button is displayed
in the case.

On the crl manager window [attachment 56139 [details]], clicking advanced would open the
edit auto update prefs window, showing current prefs, if any. Clicking update
tries to import the crl directly, using the last fetched utl, and the behavoiur
is same as to when its being updated by clicking on an url.

Comment 5

[Rangan Sen]

Note on attchment 56135 : The CN for the issuer cert is not displayed here. In
fact, for such certs, I always found CN entry to be missing - not sure if its
because they are associated to org units rather than individuals etc.

Comment 6

[Rangan Sen]

Attachment: Error message when manual import [clicking of url, or update button on crl manager] fails

Comment 7

[Rangan Sen]

Attachment: Auto update prefs displays the error details - no of successive errors and details of last error

Comment 8

[Stephane Saux]

adding Terry Hayes to the cc list.

Comment 9

[Rangan Sen]

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

Comment 10

[Rangan Sen]

OK, here's the patch....

A point to note: URL for autodownload can either be the last fetched one, or the
one advertised by ca - the later being a crl extension. Right now, NSS does not
interpret crl extensions [bug# 110383], and crlentry object will have a null
entry for this field. However, the code is written in a way to handle advertised
url, if one is available - this would not cause any significant increase in the
code, and the application logic would not require change once we start getting
the advertised url. Only in pref-crlupdate, we are NOT displaying advertised url
in the menu, because, for now, selecting that would always display <Not Define>
in the url string textbox. 

Comment 11

[Rangan Sen]

Attachment: patch

Comment 12

[Rangan Sen]

David, Javi, Kai .... Please review.

Comment 13

[Rangan Sen]

Attachment: updated version of CRL download window

Comment 14

[Rangan Sen]

Stephane, I am not too sure if we would need it for these UIs, but it might be
very useful if someone from the ui team could comment on the UI [screenshots
attached] and especially on the alert/error messages. I am not very sure about
whom to cc for that purpose, either.

Comment 15

[Kai Engert [:KaiE:]]

Rangan, please attach a unified diff that you have created using "diff -u",
which shows the surrounding areas of the changed code. This makes reviewing much
easier. It also helps the patch program, because it can work correctly, even if
the original file was meanwhile changed by someone else. I will continue to
review the rest of your changes when you have attached the new patch.

But are here some first comments already.

I wonder if the buttons in attachment 56135 [details] should be yes/no instead?

You write:
"If the crl already exists, and this is just a update, the 'download
successful' window does not the display the 'Do you want to enable auto update'
question, and clicking OK would close the window. No cancel button is displayed
in the case."

I wonder why you want to make that exception? I'd say it is fine to ask again.
If you display that dialog each time anyway, the additional option in that
dialog does not hurt. This is especially true for people who upgrade to a newer
version of the browser. Those people might already have several CRLs. Asking the
questions always allows them to discover your new feature more easily.

I wonder whether this dialog should look a bit different, just an idea: Leave
the dialog mostly unchanged, but do not ask the question "Do you want to enable
automatic update?", but make this a checkbox instead, that says "Enable
automatic update for this CRL". If you are downloading a CRL that you already
have, the check box is set, depending on what the user decided before. If you
are downloading a new CRL, I'm not sure whether it should be on or off by
default. If you decide to implement it that way, please ask around what people
think.


Maybe you do that already, I have not looked: In automatic update mode, please
do not try to download, when the user has switched to offline mode. This
prevents showing an error message while offline. 


But now to code review:

- in crlImportDialog.js /.xul, can you rename the function openPrefs to doOK? We
would have to change that anyway when we work on the patch to use overlays for
dialog buttons instead.

- can you protect the line "crl=isupport.QueryInterface" with "if (isupport)" ?

- Instead of doing  if (str == "")   I suggest to test  if (str.length == 0)  or
even simpler  if (!str.length)
  This produces better code, because no temporary "" string must be produced at
runtime.
  
  And for simple checks like  if (val == true)   I suggest to just write  if (val)
  and for                     if (val == false)  you can use              if (!val)

- You use a string property named "undefined". I'd prefer if you used a name
that describes the purpose, to allow for better identification where this string
property is used.

- The strings autoupdateURLTypeString and autoupdateURLString are only used when
the updateEnabled preference is true.
  If you move the creation of those strings into the if block, the program will
run faster and use less memory, if update is disabled.

- In the crlImportDialog.js, you update the settings in the preferences in the
onLoad function.
  I think this is not optimal. For example, when an embedder decides to
re-implement the dialog without XUL, they have to re-implement the additional
functionality, too. I think the dialog should only display the information, and
return the user's decision. Instead, the code that automatically updates the
auto-update location should be in the code that downloads the certificate. Doing
it there would even put that logic into the C++ portions of the code, which will
make the code faster.

Comment 16

[Rangan Sen]

Attachment: Patch - created with -u

Comment 17

[Rangan Sen]

Kai, Here's the new patch - with the -u. PL note this does not have your review
comments in it - I will put in a patch with all the review comments  once you
are done with rest of the review.

Regarding the window, the present crl download window is actually attachment
58044 [details] - the older one with ok/cancel was replaced after the demo. In fact, I
should have made it obsolete, which I forgot to do. This window has yes/no.
About the checkbox - as we already discussed, if we put a checkbox, and user
downloads the new crl, checks the box, and hits yes/ok, we have to open the
prefs window, which is bit unusual behaviour for checkboxes ...

Right now, even if the user is offline, we still try to update his crl. This
would not display any poop up dialog, but increse his auto update error count.
Next time he opens the auto update prefs dialog, he would see a 'Network Error'
message. Of course the error messgeas for network errors need to be more fine
grained. Right now, all of them would show network error. BUT, If we do not try
at all, the user never knows that this crl is past its due auto update time, but
failed to get updated because he was offline. Anyway, if there is such a
failure, it would be attempted again when browser restarts.

The string prop undefined actually defines the string - undefined=<Not Defined>.
This is used if we have some prop val not available - eg, next update date, for
some crls ...

Comment 18

[Kai Engert [:KaiE:]]

1)
I think one should be reluctant using try/catch blocks. For example, in
crlManager.js, in DeleteCrlSelected, you use two nested try/catch blocks. I
wonder what conditions make the outer block necessary. I would prefer to check
whether "i" has a valid value, i.e. there is something selected. If not, it
would make sense to just "return". When using that strategy, you have ensured
that i is a valid selection index, and the other calls should not fail, and the
try/cache block could be removed. One reasons why I don't like try/catch blocks,
that do nothing in the catch block: You have the risk that you don't recognize
when something goes wrong.

2)
You make a check whether GetBoolPref returns something "!= null". I'd rather not
check a bool value for != null. Just use "if (GetBoolPref())" which is the same
as "if (GetBoolPref() != false)".

3)
You are using getService(nsIPKIParamBlock). This is dangerous, as this says:
give me a pointer to an already allocated parameter block", which will give you
possibly a reference to some other open dialog's parameter block. And if you
modify it, you mess it up. You must use createInstance instead, to request a new
separate parameter block. In general, if you need to decide, whether you should
use getService, or createInstance, ask yourself: "Is there only only global
object for this interface, and I need to access functionality from it?". For
example, if you need the preferences service, there is only one, and you use
getService. But when you want to a private copy of an object, use createInstance.

4)
Whenever you query selectedIndex, you should test, whether you really received a
valid index. It is possible that a function is reached, although nothing was
selected at that time. You should only continue with your code if the selection
was valid.

5)
You have a method toggleSelectedTiming. I would suggest to find a different name
for it. When I read the code without knowing what the function does, I would
assume, it toggles, i.e. look what was the old setting, and switches to a
different setting. But this is not what the function does. I would avoid the
word toggle, maybe you could use something like updateSelectedTimingControls and
updateSelectedURLControls.

6)
Please ignore my previous request to not make preferences settings from the JS
code. I now think you are right in doing it that way.

7)
Typo: You wrote "non-negetive", but it should be "non-negative".

8)
Please remove the commented code from CrlImportStatusDialog.

9)
In pipnss.properties, you changed PrivateTokenDescription and InternalToken to
"Test Security Device". Is that what you want?

10)
In nsNSSCertificateDB::ImportCRL, after do_GetService, you removed the test
failure. If there is no good reason to remove it, I suggest to leave that check in.

11)
You have the statement "crlEntry = new nsCrlEntry(crl);", but you never free
crlEntry. This creates a leak. We must avoid that. Please make sure that you
delete it when no longer needed, i.e. "delete crlEntry;".
Please check all places where you use "new" to allocate an object, and ensure
you delete the objects.

12)
Something similar needs to be done for your statement "new
nsNSSCertificate(caCert)". After CrlImportStatusDialog returned, you must free
that variable. However, it is too dangerous to free it directly, because you
give that object to JavaScript. The dialog could run on a separate thread, and
JavaScript might still refer to the object when you return. Therefore, you must
use nsCOMPtr to do it correctly.
I suggest, instead of

  certDialogs->CrlImportStatusDialog(cxt, new nsNSSCertificate(caCert), crlEntry);

do

  nsCOMPtr<nsIX509Cert> x509cert = new nsNSSCertificate(caCert);
  certDialogs->CrlImportStatusDialog(cxt, x509cert, crlEntry);

The destructor of nsCOMPtr will decrement the reference count and delete the
object if already reached zero. If not, it will be free when JavaScript does no
longer reference the object.

13)
You have code "nsCString *... = new nsCString ...". You don't free that string
objects that you allocate on the heap. This will create another leak.
But you don't need to allocate the strings on the heap, I don't see that you
pass the string pointer a method. I think you can simply use a stack based auto
variable.
Instead of
  nsCString *updateErrCntPrefStr = new nsCString(CRL_AUTOUPDATE_ERRCNT_PREF);
just write
  nsCString updateErrCntPrefStr(CRL_AUTOUPDATE_ERRCNT_PREF);
And when you use 
  updateErrCntPrefStr->
just do
  updateErrCntPrefStr.

14)
Instead of
  dayCnt = (new nsCString((const char *)dayCntStr))->ToFloat(&errCode);
you can just write
  dayCnt = atof(dayCntStr);

15)
I see in several places that you treat the number of days setting as a float
value. Do you want that? Or should we use integers instead? In that case, you
could replace parseFloat with parseInt, and in 14) instead of atof use atol.

16)
NSS functions that return you a string buffer, like CERT_GetOrgUnitName,
transfer ownership over that buffer to you. You are responsible to free that
buffer when you no longer need it, or we have a leak. Use PORT_Free to free
pointers you received from NSS. I don't if there is a general rule, which
function must be used to free objects, I saw PORT_Free is used in some other
places. In doubt please ask Bob Relyea for advice.

16)
Suggestion: I prefer to have checks for success, i.e. whether we actually
received a non-zero pointer, when do_GetService or do_CreateInstance was called.
I also prefer, when a function receives a pointer as an argument, the function
should check whether the pointer is non-zero.

16)
In UpdateCRLFromURL, you do

	if(NS_FAILED(rv))
	    *res = NS_ERROR_FAILURE;
	else
	    *res = NS_OK;

	return rv;

But parameter res is of type PRBool. Note that this type is not designed to
store error codes, it should only be used to store true or false. It depends
what you want to see in PRBool. But I assume you want to see the answer to the
question "was the operation successful?". In that case you would write:

	if(NS_FAILED(rv))
	    *res = PR_FALSE;
	else
	    *res = PR_TRUE;

	return rv;

But being even more specific, you could do something completely different. You
function has two return values. the real return code, which is of type nsresult,
and is what you give back in the return statement. You can see in the IDL
definition, that this type is not included in what you defined. It is used
internally by XPCom. For example, if that function is called from JavaScript,
the environment will not give this nsresult code to your JavaScript code, but
the JavaScript engine checks for this return code. If it sees anything else but
NS_OK, an exception is triggered, and that is why you we sometimes use try/catch
blocks. In normal conditions, your function should return NS_OK. You should
return a failure when you detect an error, for example, when do_GetService gave
you a null pointer. In the boolean result code, just return the result code of
your own action, i.e. either true or false.

17)
I don't like the method name nsNSSComponent::DownloadFromURL. The method is in a
very general class, and the name should indicate that you are downlading a CRL,
because the method behaviour is fixed to that.

18)
In the same method, I suggest you avoid the two casts. Instead of
	nsIStreamListener *psmDownloader = new
PSMContentDownloader(PSMContentDownloader::PKCS7_CRL);
	((PSMContentDownloader *)psmDownloader)->setSilentDownload(PR_TRUE);
	((PSMContentDownloader *)psmDownloader)->setCrlAutodownloadKey(mCrlUpdateKey);
use
	PSMContentDownloader *psmDownloader = new
PSMContentDownloader(PSMContentDownloader::PKCS7_CRL);
	psmDownloader->setSilentDownload(PR_TRUE);
	psmDownloader->setCrlAutodownloadKey(mCrlUpdateKey);
The other code in the method will still work.

18)
In nsNSSComponent.h, please remove the //void Notify

19)
To prevent memory corruption, and to enhance problem detection, you should add
code to the constructor of nsNSSComponent, that inits crlsScheduledForDownload
with nsnull.

20)
Please delete crlsScheduledForDownload.

21)
We must make sure that the crlsScheduledForDownload and all timers are deleted
when we receive the event that the user profile switches.

22)
Please remove the debug printf from nsNSSComponent::InitializeNSS.
One hint: This method already contains some debug statement. In your
environment, define NSPR_LOG_MODULES="pipnss:5" and you will see that trace
messages.

23)
Please remove the commented old PSMContentDownloader from nsNSSComponent.cpp.

24)
Suggestion: Wherever you add a "to do" comment, you could add the text "XXX".
This is some kind of convention to mark places in the source where more work is
needed.

Comment 19

[Rangan Sen]

Kai, Here's the new patch. Most of your concerns are addressed, except :

#1. I have compacted the two nested try's into one. But, here we cannot 
eliminate the nested try blocks completely. Get..Pref(..) would throw an 
exception if the pref is not available/malformed, and so does clearUserPref(..). 
But, we do not want delete to fail if there is no prefs defined for the crl 
[Note that the prefs.js would not have any prefs for a crl that was never 
activated for autoupdate.] The outer block would handle in case there is some 
error in actually deleting the crl, etc..

#11. Rather that delet in the crlEntry object, I am using nsCOMPtr<nsICrlEntry> 
here as well...this is being passed to js

#12. new nsNSSCertificate(caCert) was actually redundant - for this was not 
being used anywhere is the importcrl dialog. Removed it completely.

#15. This was intentional - the idea was to allow the user to have maximum 
flexibility to define crl update timing/freq.

#16. Null pointer check - In general, I agree. But in some cases, like reading 
the download url from the prefs and importing the crl - where getting a 
null/blank url etc, would might imply some serius trouble with the prefs data, 
and the user should be informed about the error when the opens his seting window 
- it might be useful to rather let the null pointer error travel upto the actual 
error handler, rather than checking it right at the beginning, for in  that 
case, we either need to invoke error handling code at that point, or just return 
from the function with an error. It these cases it might be an elegent solution 
to invoke the downloader, get a invalid url error, and update the error string. 
Also, in some cases, some functions are tightly coupled, and meant to be invoked 
in a certain sequence, which itsely guarantees that a null pointer would not be 
passed in - in such cases a check would be redundant.    

#20+21: Reseting and deleting hashtable in destructor. Refering to yesterdays 
discussion on IRC, for the nsHashtable, using Reset would probably do the job, 
and enumeration will not be required, as in PL_Hashtable. That's the way I found 
it being used in many places in mozilla - I guess it because the keys in this 
case has to be nsHashKey object ... I am not too sure about the reason, though 
...but looks like it works ok..

Patch Follows ....

Comment 20

[Rangan Sen]

Attachment: updated patch - incorporates Kai's suggestions - Pl review, Kai

Comment 21

[Rangan Sen]

Attachment: updated patch - sorry, wrong patch uploaded accidentally

Comment 22

[Rangan Sen]

Attachment: Correct one this time

Comment 23

[Kai Engert [:KaiE:]]

In StopCRLUpdateTimer, please cancel and delete the timer first, before you
delete the scheduled CRLs, in order to prevent possible crashes.

After
  delete crlsScheduledForDownload;
please add
  crlsScheduledForDownload = nsnull;
This prevents the possibility that the invalid pointer could be used or deleted
again.

You need to init mTimer to nsnull in the constructor to prevent crashes.
Especially as you check for !mTimer in DefineNextTimer, where no check for
mUpdateTimerInitialized is made. Note that in C++ you can not rely on variables
to be initialized to zero. You always need to do it yourself.

Please clean up the spacing and remove all "hard tabs" from your changes. Please
configure your editor to not use hard tabs, but to emulate tabs using spaces.
Set tabs to 2 spaces. When in doubt you can look at the first line of source
files, which is the "mode line", which is present in many files and tell you the
tab modes within that file.

Please do the above, and you have r=kaie.

Comment 24

[Rangan Sen]

Attachment: update patch - with kaie's latest comments

Comment 25

[Kai Engert [:KaiE:]]

Comment on attachment 60194 [details] [diff] [review]
update patch - with kaie's latest comments

Just note that in nsNSSComponent::~ you still delete the crls first, but you
should cancel the timer first. Thanks.

Comment 26

[Darin Fisher]

Comment on attachment 60194 [details] [diff] [review]
update patch - with kaie's latest comments

i haven't looked at any of the JS/XUL/DTD code changes... you'll want to get
an sr= from someone more knowledgeable in that area.



>Index: pki/src/nsNSSDialogs.cpp

>+NS_IMETHODIMP 
>+nsNSSDialogs::CrlImportStatusDialog(nsIInterfaceRequestor *ctx, nsICrlEntry *crl)
>+{

>+}
>+
>+
>+

extra spaces ok?



>Index: ssl/public/nsINSSDialogs.idl

>Index: ssl/public/nsIX509CertDB.idl

>+  /*
>+   * update crl from url
>+   * update an existing crl from the last fteched url. Needed for the update
>+   * button in crl manager
>+   */
>+  boolean updateCRLFromURL(in wstring url, in wstring key);

typo "fteched"



>Index: ssl/resources/locale/en-US/pipnss.properties

>Index: ssl/src/Makefile.in

> 		  content \
> 		  pippki \
> 		  xpconnect \
>+             timer \
> 		  jar \
> 		  unicharutil \
>                   pipboot \

fix indentation.


>Index: ssl/src/makefile.win

> 		jar \
> 		layout_xul \
> 		gfx \
>+           timer \
> 		unicharutil \
>                 pipboot \
> 		$(NULL)

here as well.


>Index: ssl/src/nsNSSCertificate.cpp

>+      if(importSuccessful){

these should probably be nsCAutoString's instead.

>+          nsCString updateTypePrefStr(CRL_AUTOUPDATE_TIMIINGTYPE_PREF);
>+          nsCString updateTimePrefStr(CRL_AUTOUPDATE_TIME_PREF);
>+          nsCString updateUrlPrefStr(CRL_AUTOUPDATE_URL_PREF);
>+          nsCString updateUrlTypePrefStr(CRL_AUTOUPDATE_URLTYPE_PREF);
>+          nsCString updateDayCntPrefStr(CRL_AUTOUPDATE_DAYCNT_PREF);
>+          nsCString updateFreqCntPrefStr(CRL_AUTOUPDATE_FREQCNT_PREF);


>+      } else{

these should also be nsCAutoString's instead.

>+          nsCString errMsg;
>+          nsCString updateErrDetailPrefStr(CRL_AUTOUPDATE_ERRDETAIL_PREF);


>+	//Now, a basic constraing is that the next auto update date can never be after
>+	//next update, if one is defined
>+	if(LL_CMP(mNextUpdate , > , 0 )) {
>+       if(LL_CMP(tempTime , > , mNextUpdate)) {
>+          tempTime = mNextUpdate;
>+       }
>+    }

indentation problems.


>+
>+	nsAutoString nextAutoUpdateDate;
>+	nsString tempBuf;

how about make |tempBuf| a nsAutoString?  actually, what is the point of
|tempBuf|?


>+	PRExplodedTime explodedTime;
>+	nsresult rv;
>+	nsCOMPtr<nsIDateTimeFormat> dateFormatter = do_CreateInstance(kDateTimeFormatCID, &rv);
>+	if (NS_FAILED(rv))
>+		return rv;
>+	PR_ExplodeTime(tempTime, PR_GMTParameters, &explodedTime);
>+	dateFormatter->FormatPRExplodedTime(nsnull, kDateFormatShort, kTimeFormatSeconds,
>+                                      &explodedTime, nextAutoUpdateDate);
>+	tempBuf.Assign(nextAutoUpdateDate.get());
>+	*nextAutoUpdate = ToNewUnicode(nextAutoUpdateDate);
>+
>+	return NS_OK;
>+}


>+NS_IMETHODIMP 
>+nsNSSCertificateDB::UpdateCRLFromURL( const PRUnichar *url, const PRUnichar* key, PRBool *res)
>+{
>+    nsresult rv;
>+    nsString downloadUrl(url);
>+    nsString dbKey(key);

nsAutoString?  do you really need to copy the input string?  if not, then
consider using
nsDependentString to reference |url| and |key|.


>+    nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
>+	if(NS_FAILED(rv)){
>+	   *res = PR_FALSE;
>+	   return rv;
>+	}
>+
>+    rv = nssComponent->DownloadCRLDirectly(downloadUrl, dbKey);
>+	if(NS_FAILED(rv)){
>+	   *res = PR_FALSE;
>+	} else {
>+	   *res = PR_TRUE;
>+	}
>+    return NS_OK;
>+
>+}

indentation problems.


>+NS_IMETHODIMP 
>+nsNSSCertificateDB::RescheduleCRLAutoUpdate(void)
>+{
>+	nsresult rv;
>+	nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
>+	if(NS_FAILED(rv)){
>+	   return rv;
>+	}
>+	rv = nssComponent->DefineNextTimer();
>+	return rv;
>+
>+}

indentation/whitespace problems.


>Index: ssl/src/nsNSSCertificate.h

>Index: ssl/src/nsNSSComponent.cpp

>@@ -198,6 +219,24 @@
> {
>   PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("nsNSSComponent::dtor\n"));
> 
>+  if(mUpdateTimerInitialized == PR_TRUE){
>+     if(crlsScheduledForDownload != nsnull){
>+	   crlsScheduledForDownload->Enumerate(crlHashTable_clearEntry);
>+        crlsScheduledForDownload->Reset();
>+        delete crlsScheduledForDownload;
>+	 }
>+
>+     PR_Lock(mCrlTimerLock);
>+	 if(crlDownloadTimerOn == PR_TRUE){
>+		mTimer->Cancel();
>+	 }
>+	 crlDownloadTimerOn = PR_FALSE;
>+	 PR_Unlock(mCrlTimerLock);
>+	 PR_DestroyLock(mCrlTimerLock);
>+
>+	 mUpdateTimerInitialized = PR_FALSE;
>+  }

indentation/whitespace problems.


>+nsresult
>+nsNSSComponent::DownloadCRLDirectly(nsString url, nsString key)
>+{
>+	//This api is meant to support dierct interactive update of crl from the crl manager
>+	//or other such ui and potentially would be called from a different context. So, we

>+	rv = NS_OpenURI(psmDownloader, nsnull, pURL);
>+	return rv;
>+
>+}

indented too much?  all of these functions seem to have inconsistent
indentation:

>+nsresult nsNSSComponent::DownloadCrlSilently()

>+nsresult nsNSSComponent::getParamsForNextCrlToDownload(nsString *url, PRTime *time, nsString *key)

>+void nsNSSComponent::Notify(nsITimer *timer)

>+nsNSSComponent::RemoveCrlFromList(nsString key)

>+nsNSSComponent::DefineNextTimer()

>+nsNSSComponent::StopCRLUpdateTimer()

>+nsNSSComponent::InitializeCRLUpdateTimer()




>@@ -1040,6 +1372,8 @@
>       }
>     }
>     
>+	StopCRLUpdateTimer();
>+

indentation?

>+
>+	InitializeCRLUpdateTimer();

>   : mByteData(nsnull),
>-    mType(type)
>+    mType(type),
>+	mDoSilentDownload(PR_FALSE)

indentation?


summary: lot's of indentation/whitespace problems... i don't think we should be
checking in code like this.  please revise.

Comment 27

[Rangan Sen]

Darin : Here's the new patch. Following things have been fixed:

1. The indentation/tab problem - Somehow I was messing this up everytime... but 
I trust I have got it completely fixed this time. Sorry about the mess

2. "how about make |tempBuf| a nsAutoString?  actually, what is the point of
   |tempBuf|?" - tempBuf was actually redundant - removed

3. Auto versions have been used in place of nsString and nsCString excluding the 
places the follwoing places: 

A.
>+NS_IMETHODIMP 
>+nsNSSCertificateDB::UpdateCRLFromURL( const PRUnichar *url, const PRUnichar* 
key, PRBool *res)
>+{
>+    nsresult rv;
>+    nsString downloadUrl(url);
>+    nsString dbKey(key);

nsAutoString?  do you really need to copy the input string?  if not, then
consider using nsDependentString to reference |url| and |key|.

This function is meant to be called from js, potentially on a diff thread. And 
call to UpdateCRLFromURL finally starts off an asynchronous download [which also 
would be on a diff thread] using the url. Thats the reason why copy was used in 
both this case, as well as DownloadCRLDirectly and DownloadCrlSilently methods 
in NSSComponent. 

B. nsString is used when readinging String props ... a ref is passed.

C. The mDownloadURL and mCrlUpdateKey properties in NSSComponent class.

Kaie: Your latest comment [seqence of deleting hashtable entries and 
canceling the timer] has been incorporated in this update

Comment 28

[Rangan Sen]

Attachment: updated patch

Comment 29

[Rangan Sen]

CC-ing Joe for sr= on the xul/js part

Comment 30

[Darin Fisher]

rangan: regarding point A .. if you need to make a copy of a string, prefer
nsAutoString over nsString.  nsAutoString can be used anywhere a nsString can be
used, but assignment into a nsAutoString is often much much cheaper.  there
shouldn't be any difference from a threading point-of-view.

Comment 31

[Darin Fisher]

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

>+nsresult nsNSSComponent::DownloadCrlSilently()
>+{
>+  nsresult rv;
>+  nsIURI *pURL;
>+  PRTime now = PR_Now();

|now| is never used.


>+  //Check if the download succeeded - it might have failed due to
>+  //network issues, etc.
>+  if (NS_FAILED(aStatus)){
>+	  handleContentDownloadError(aStatus);
>+	  return aStatus;
>+  }

indentation nit ;)

Comment 32

[Rangan Sen]

It says that nsAutoString allocates memmory for the buff from stack. So, in 
cases as these, where we would be invoking functions potentially on a diff 
thread, is it not unsafe to use nsAutoString?

Comment 33

[Rangan Sen]

I was not very specific about the last question - let me rephrase it -
Before I invoke NS_OpenURI, I create the nsIURI obj with NS_NewURI, and I pass 
in a nsCString here rather than the nsCAutoString. Now, since for nsCautoString, 
the buff is assigned from the stack and I am basically starting off an 
asynchronous d/l, potentially on a diff thread, would it not be unsafe if I had 
actually used nsCAutoString in this case?

Rest of the places [excluding B] I agree I should be using nsCAutoString.

Comment 34

[Darin Fisher]

first of all: each thread has its own unique stack, so it wouldn't matter if you
allocated memory on the stack or in the heap.

secondly: you should not be calling NS_OpenURI from a thread other than the UI
thread.  necko is not thread safe.

Comment 35

[Joe Hewitt (gone)]

* In many places you are setting value, disabled, and hidden as attributes. Set
them as jsproperties instead.

* In pref-crlupdate.xul you should be using the dialog tag, not rolling your own
dialog buttons. 

Comment 36

[Rangan Sen]

Attachment: new patch -with Hewitt's and Darin's comments

Comment 37

[Joe Hewitt (gone)]

in the new patch, pref-crlupdate.xul is still not using the dialog tag.  You
should really convert it, you can make your xul file a little bit leaner
(Removing those ok/cancel/help buttons) and get keyboard enter/escape handling
for free.

Comment 38

[Darin Fisher]

Comment on attachment 60407 [details] [diff] [review]
new patch -with Hewitt's and Darin's comments

sr=darin (on the C++ and .idl changes)

Comment 39

[Rangan Sen]

Attachment: update - with hewitts comments.

Joe: dialog class is used this time - in fact for both the new xul files -
pref-crlupdate.xul and crlImportDialog.xul. As for seplacing setAttributes by
js properties, it has been replaced in all cases but two - 
1. for text Elements - using var.value does not seem to work - I have seen this
in a few other cases [bug# 1090722, for example]- possibly a xul bug.
2. The same thing for menu labels.
In other cases, properties are directly used.

Comment 40

[Rangan Sen]

Sorry, typo that Bug #109072

Comment 41

[Rangan Sen]

Actually, that's the reverse problem - in that case, using setAttribute did not 
work, but .value did, but here I find the using .value does not work, and 
setAttribute does - that's rather weired...

Comment 42

[Joe Hewitt (gone)]

Rangan, I am asking you to use the dialog tag, <dialog>, not dialogOverlay.xul.
 That overlay has been deprecated and will soon be banished from the tree. 
Please use <dialog> instead of <window>.  There are plenty of examples of how to
use this new tag in the tree.

Comment 43

[Rangan Sen]

Attachment: Patch with <dialog> replacing <window> tag

Dialog tag used for crlImportDialog.xul as well - with ok/cancel button texts
overridden with yes/no.

Comment 44

[Joe Hewitt (gone)]

Comment on attachment 60583 [details] [diff] [review]
Patch with <dialog> replacing <window> tag

Nice job, Rangan.
sr=hewitt

Comment 45

[Rangan Sen]

Patch checked in ...

Comment 46

[Rangan Sen]

A Few Points to note:
This patch enables automatic crl update; but some issues with crl are still 
open:
* The issues related to crl next update for crl import [bug# 99964] and 
  validation against crl [bug# 98193]. Integration with NSS 3.4 would be  
  required to resolve these.
* Delta CRL issues - Browser still allows them to be downloaded [Bug# 103946]

Comment 47

[Rangan Sen]

cc-ing Sean

Comment 48

[John Unruh]

Verified fixed.