Closed Bug 581175 Opened 10 years ago Closed 10 years ago

NetUtil's asyncFetch should suppress SSL error dialogs by default

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b3

People

(Reporter: Gavin, Assigned: Gavin)

References

Details

Attachments

(1 file, 3 obsolete files)

Currently, attempts to asyncFetch resources with invalid SSL or SSL cert configs throws up the default dialog. That should be preventable, ideally.

I'm doing this using an optional "options" JS object parameter, since I foresee other options potentially being useful here (bypassCache and/or loadFlags are two that come to mind).
Depends on: 581176
Attached patch patch (obsolete) — Splinter Review
This also passes the aRequest parameter to the callback so that it can do fancy stuff if desired. I had to test with a browser chrome test since SSL support in xpcshell tests doesn't exist yet. It has no Firefox dependencies AFAICT so it should run fine in e.g. SeaMonkey.
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #459590 - Flags: review?(sdwilsh)
(In reply to comment #1)
> SSL support in xpcshell tests doesn't exist yet

(bug 466524)
Though now that I think about it a bit more, perhaps this should just be the default behavior? Not sure any callers expect this to potentially throw up modal dialogs...
Drive-by comments:
1) didn't update the documentation for what the callback gets
2) We should import CertUtils lazily probably - no point in paying that price all the time
3) Comment on what true does in BadCertListener would be helpful.
4) Wouldn't a mochitest-chrome test be better so it doesn't depend on the browser?

(In reply to comment #3)
> Though now that I think about it a bit more, perhaps this should just be the
> default behavior? Not sure any callers expect this to potentially throw up
> modal dialogs...
Yeah, likely.
(In reply to comment #4)
> 1) didn't update the documentation for what the callback gets

I disagree!

> 2) We should import CertUtils lazily probably - no point in paying that price
> all the time

Will do.

> 3) Comment on what true does in BadCertListener would be helpful.

At the call site, you mean? Can do.

> 4) Wouldn't a mochitest-chrome test be better so it doesn't depend on the
> browser?

How is mochitest-chrome different than browser-chrome in that regard? As I said, there are no Firefox dependencies in the test.
Comment on attachment 459590 [details] [diff] [review]
patch

(In reply to comment #5)
> (In reply to comment #4)
> > 1) didn't update the documentation for what the callback gets
> 
> I disagree!
You updated half the documentation, but missed this part:
>      * @param aCallback
>      *        The callback function that will be notified upon completion.  It
>      *        will get two arguments:
>      *        1) An nsIInputStream containing the data from the channel, if any.
>      *        2) The status code from opening the source.

> At the call site, you mean? Can do.
Yes call site please.

> How is mochitest-chrome different than browser-chrome in that regard? As I
> said, there are no Firefox dependencies in the test.
Thought mochitest-chrome was the only thing all apps run.  Maybe that's changed though...
Summary: Add suppressSSLErrors option to NetUtil's asyncFetch → NetUtil's asyncFetch should suppress SSL error dialogs by default
Attached patch updated patch (obsolete) — Splinter Review
Attachment #459590 - Attachment is obsolete: true
Attachment #459936 - Flags: review?(sdwilsh)
Attachment #459590 - Flags: review?(sdwilsh)
Attached patch real updated patch (obsolete) — Splinter Review
Attachment #459936 - Attachment is obsolete: true
Attachment #459937 - Flags: review?(sdwilsh)
Attachment #459936 - Flags: review?(sdwilsh)
SeaMonkey and Fennec both run the browser-chrome tests.
Comment on attachment 459937 [details] [diff] [review]
real updated patch

For detailed review comments, please see http://reviews.visophyte.org/r/459937/.

on file: netwerk/base/src/NetUtil.jsm line 139
>      *        Note: if passing an nsIChannel whose notificationCallbacks is

nit: If not if please :)


on file: netwerk/base/src/NetUtil.jsm line 185
>           if (typeof(BadCertHandler) == "undefined")
>             Cu.import("resource://gre/modules/CertUtils.jsm");

I was actually expecting a global lazy getter (see end of file).  Would prefer
that, but no strong feelings.  If you don't go that way, please brace the if
like the rest of the file (all ifs have bracing).


on file: netwerk/base/src/NetUtil.jsm line 187
>           // pass true to avoid optional redirect-cert-checking behavior

nit: capitalization + punctuation at the end please


on file: netwerk/test/browser/browser_NetUtil.js line 1
> /* ***** BEGIN LICENSE BLOCK *****
>  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>  *
>  * The contents of this file are subject to the Mozilla Public License Version
>  * 1.1 (the "License"); you may not use this file except in compliance with
>  * the License. You may obtain a copy of the License at
>  * http://www.mozilla.org/MPL/
>  *
>  * Software distributed under the License is distributed on an "AS IS" basis,
>  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>  * for the specific language governing rights and limitations under the
>  * License.
>  *
>  * The Original Code is NetUtil test code.
>  *
>  * The Initial Developer of the Original Code is
>  * the Mozilla Foundation.
>  * Portions created by the Initial Developer are Copyright (C) 2010
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>  *   Gavin Sharp <gavin@gavinsharp.com>
>  *
>  * Alternatively, the contents of this file may be used under the terms of
>  * either the GNU General Public License Version 2 or later (the "GPL"), or
>  * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>  * in which case the provisions of the GPL or the LGPL are applicable instead
>  * of those above. If you wish to allow use of your version of this file only
>  * under the terms of either the GPL or the LGPL, and not to allow others to
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */

Didn't want to go with the public domain block, eh?
(http://www.mozilla.org/MPL/license-policy.html)


on file: netwerk/test/browser/browser_NetUtil.js line 54
>   test_asyncFetchBadCert

nit: trailing comma please


on file: netwerk/test/browser/browser_NetUtil.js line 63
>     // Close the dialog

In general, comments should be proper sentences please! (will stop commenting
on this)


r=sdwilsh with comments addressed.
Attachment #459937 - Flags: review?(sdwilsh) → review+
Attached patch for checkinSplinter Review
Attachment #459937 - Attachment is obsolete: true
Attachment #460924 - Flags: approval2.0?
Attachment #460924 - Flags: approval2.0? → approval2.0+
https://hg.mozilla.org/mozilla-central/rev/da01163529c3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
Depends on: 589609
(In reply to comment #5)
> (In reply to comment #4)
> > 4) Wouldn't a mochitest-chrome test be better so it doesn't depend on the
> > browser?
> How is mochitest-chrome different than browser-chrome in that regard? As I
> said, there are no Firefox dependencies in the test.
mochitest-chrome makes it easier to avoid inadvertent Firefox dependencies, e.g. using browser-specific globals such as Cu.
You need to log in before you can comment on or make changes to this bug.