287648 - Unify 'request' object interface for DNS and PPS asyncResolve methods

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Darin Fisher]

Unify 'request' object interface for DNS and PPS asyncResolve methods

nsIProtocolProxyService::asyncResolve returns a nsISupports request object that
can be canceled via nsIProtocolProxyService::cancelAsyncResolve.

nsIDNSService::asyncResolve returns a nsIDNSReqeuest object that can be canceled
via nsIDNSRequest::cancel.

These should be handled similarly.  Maybe they should each return an object with
a cancel method, or maybe they should each move the cancel method onto the
service interface.  Another approach involves implementing nsIRequest, but that
is very heavy weight for these objects.  See bug 282442 comment #43.

Comment 1

[Darin Fisher]

Attachment: v1 patch

OK, I went ahead with the nsIProtocolProxyRequest solution.  I think this is
cleaner and easier for consumers than cancelAsyncResolve.  The extra interface
is not that bad.  As for a unified interface shared between the DNS and PPS:
*shrug*
At least the programming paradigm is the same.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 178888 [details] [diff] [review]
v1 patch

+	 // Provided we haven't been canceled...
+	 if (mStatus == NS_OK) {

not NS_SUCCEEDED?

Comment 3

[Darin Fisher]

> (From update of attachment 178888 [details] [diff] [review] [edit])
> +	 // Provided we haven't been canceled...
> +	 if (mStatus == NS_OK) {
> 
> not NS_SUCCEEDED?

This code is testing to see if Cancel was called.  Since we initialize mStatus
to NS_OK, I figured I'd test for NS_OK.  There's no point in testing for the
more generic NS_SUCCEEDED.  The alternative would be to test for NS_ERROR_ABORT.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 178888 [details] [diff] [review]
v1 patch

>Index: base/public/nsIProtocolProxyRequest.idl

I almost wonder whether we should use the same nsIBasicCancelableRequest
interface for both this and the DNS service... It seems silly to have two
interfaces with the same exact functionality and different names.  If we really
do want the two separate interfaces for some reason, perhaps they should both
inherit from nsIBasicCancelableRequest or something?

>Index: base/src/nsProtocolProxyService.cpp
>     nsresult DispatchCallback()

It's probably best to null out mCallback if this method ends up throwing;
otherwise I don't see any way for the reference cycle to be broken...

>     void OnQueryComplete(nsresult status, const nsCString &pacString)
>+        // If we've already called DoCallback then, nothing more to do.

Comma should be before "then", not after.

sr=bzbarsky with the two nits and if we're happy with the interfaces setup
as-is....

Comment 5

[Darin Fisher]

bz: thanks for the feedback.  any better name than nsIBasicCancelableRequest?

Comment 6

[Darin Fisher]

nsICancelable?

Comment 7

[Darin Fisher]

Attachment: v1.1 patch

Spoke to bz over IRC, and he seemed happy with nsICancelable.  This patch
implements that, but only for the PPS at this time.  I'm going to wack the DNS
interfaces with a subsequent patch (need to get rid of init and shutdown as
well in that patch).

Comment 8

[Darin Fisher]

Attachment: v1.1 interdiff

interdiff for easier reviewing.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 179300 [details] [diff] [review]
v1.1 patch

given:
+   *	     being canceled.  It is an error to pass a success code.

can this:
+	 NS_ENSURE_ARG(NS_FAILED(reason));
just be an NS_ASSERTION?

r=me either way

Comment 10

[Darin Fisher]

The v1.1 patch landed.  I'm preparing a followup patch that makes the DNS
service use nsICancelable as well.

Comment 11

[Darin Fisher]

Attachment: patch: make DNS service use nsICancelable (v1)

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 179785 [details] [diff] [review]
patch: make DNS service use nsICancelable (v1)

looks good. should nsPIDNSService maybe be nonscriptable?

+    // TODO(darin): make XPCOM proxies support any nsIEventTarget impl

wanna file a bug on this?

Comment 13

[Darin Fisher]

yeah, i'll file a bug on that.  no reason in my mind to make the interface
non-scriptable.  we might want to use it from a unit test for instance.

Comment 14

[Darin Fisher]

fixed-on-trunk