Closed Bug 472074 Opened 14 years ago Closed 7 years ago

Firefox 3 returns E_NOINTERFACE for nsICookieService ("@mozilla.org/cookieService;1")

Categories

(Core :: Networking: Cookies, defect)

1.9.1 Branch
x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED WONTFIX

People

(Reporter: changov, Unassigned)

References

Details

Attachments

(1 file)

User-Agent:       Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.2; .NET CLR 1.1.4322; InfoPath.2; .NET CLR 4.0.11029)
Build Identifier: Firefox/3.0.5

This is the code that works with Firefox v2.x. It is essentially a C++ translation of the sample here: https://developer.mozilla.org/en/Code_snippets/Cookies.  With v3, GetServiceByContractID() returns E_NOINTERFACE. 

nsIServiceManager* GetServiceManager()
{
    static nsCOMPtr<nsIServiceManager> spServiceManager;
    if(!spServiceManager)
    {
        NS_GetServiceManager(getter_AddRefs(spServiceManager));
    }
    return spServiceManager;
}

nsICookieService* GetCookieService()
{
    static nsCOMPtr<nsICookieService> spCookieService; 
    if(!spCookieService)
    {
        GetServiceManager()->GetServiceByContractID(
            "@mozilla.org/cookieService;1", NS_GET_IID(nsICookieService), (void**)&spCookieService); 
    }
    return spCookieService;
}

Reproducible: Always




There seems to be no published information about this being an intentional breaking change. The problem still exists with FF v3.1 Beta 2.

Due to this change, .NET/WPF browser hosted applications (XBAPs) that use HTTP cookies will likely crash when running in Firefox 3. This issue affects users of the (already released) .NET Framework v3.5.
Component: Extension Compatibility → Networking: Cookies
Product: Firefox → Core
QA Contact: extension.compatibility → networking.cookies
Version: unspecified → 1.9.1 Branch
Depends on: 383994
timeless, what does this have to do with the p3p bug?

can't think otoh what would cause this. have you tried with a new profile? could be a failure starting up the cookieservice, not sure if that'd throw E_NOINTERFACE though...
reporter: can you please explain precisely what ms .net fw 3.5 does?

if it has actually encoded the uuid, then my "patch" is pretty much what we'd have to do.

if you have the ability to ship an update, then what you should do is provide two .idl files, one named

nsICookieServiceMozilla18.idl which has the old contents (w/ nsICookieService renamed to nsICookieServiceMozilla18)
<http://mxr.mozilla.org/mozilla1.8/source/netwerk/cookie/public/nsICookieService.idl?raw=1>

and

nsICookieServiceMozilla191.idl which has the new contents (w/ nsICookieService renamed to nsICookieServiceMozilla191)
<http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/public/nsICookieService.idl?raw=1>

you'd then do something more like:

class MyWrapper : nsICookieServiceMozilla191
{
...
}


nsICookieService* GetCookieService()
{
    /* NEVER USE STATICS WITH COMPTRS THANK YOU */
    nsCOMPtr<nsISupports> basic;
    GetServiceManager()->GetServiceByContractID(
        "@mozilla.org/cookieService;1",
        NS_GET_IID(nsISupports),
        getter_AddRefs(basic)); 
    if (!basic)
        return 0;
    nsCOMPtr<nsICookieServiceMozilla18> cookies18(do_QueryInterface(basic); 
    if (cookies18)
        return new MyWrapper(cookies18);
    nsCOMPtr<nsICookieServiceMozilla191> cookies191(do_QueryInterface(basic); 
    if (cookies191)
        return new MyWrapper(cookies191);

    return 0;
}

How exactly you write MyWrapper (whether you have two distinct impls which is probably a better idea, or two distinct constructors) would be up to you.
timeless, thanks much for the instant and thorough response.

I guess I can say we have the UUID baked into our binary. (It's an NPAPI plug-in that also uses XPCOM interfaces for some more advanced things.) From the two header files you referenced, I can see how the UUID was changed. 
  - Is that common practice for "unfrozen" interfaces? 
  - Is there an alternate interface for cookie access that is considered stable?

If we have to, we could ship an update, but that'd be logistically troublesome, and we would rather see a Firefox update that makes the interface with the original UUID functional again. That would un-break existing installations of .NET v3.5. I wonder: Isn't this change in FF 3 breaking other extensions/add-ins that use cookies? (I guess the only reason that not to be the case would be that most use some other, more official/stable interface.)

I can provide further explanation of our particular usage and even simple instructions for a live repro (with .NET v3.5 installed), but the problem & cause seem clear enough already.

Thanks.
nsICookieManager may do what you need, but I can't say that for certain without knowing what you want to do...
it at least has the advantage of being frozen, which appears to be one of your requirements here, if you're embedding the uuid in your binary...
Can we count on a Firefox update to bring back the interface? (How do such things get decided in Mozilla?) If not, we'll have to produce a patch for our code.
nsICookieService is intended for internal use, and will never be frozen. in broad terms, it provides the methods required for the networking layer to interface to the cookie backend. for programmatic access to the cookie database, for embeddors and addons etc, you should be using nsICookieManager - which is frozen. nsICookieManager2 provides some additional methods which you may be interested in, but it's changing and will continue to change.

we could also roll a custom embedding API for cookies, and freeze that, but i haven't looked into that possibility...

in any case, this is going to involve patching your code... if you can talk about what you're looking for in a frozen cookie API, we can figure out what best to do here.
sorry, to answer the basic question, yes, if an interface changes we're supposed to rev the uuid, as we did here. and for unfrozen interfaces, we really make no promises, so such a change is likely.

you can use mxr/bonsai or perhaps even mdc to determine if the nsICookieManager has been around in its frozen state long enough for you to use. I suspect it has.

fwiw, in the past we've offered to review consumers' lists of used interfaces and methods. if you could provide a complete list of interfaces/properties, someone would probably be willing to review your list and highlight other similar potential problems.
Okay, lesson learned: Stay away from interfaces that don't say FROZEN! Thankfully, it appears that all the other XPCOM interfaces we've used are frozen.

However, it appears we don't have an alternative to nsICookieService presently. We need to add cookies, and setCookieStringFromHttp() is *exactly* what we need: The cookie data can come from an HTTP header. (We are kind of bridging cookie access between the Firefox and IE's UrlMon/WinINet networking stacks.) nsICookieManager can only query cookies. nsICookieManager2 can add, but's un-frozen. Even if it were frozen, we'd have to do the parsing of the cookie header data (to gather all the arguments for the add() method), which is obviously involved & risky.

So, at this point it seems we could only ship a patch with the new nsICookieService UUID and *hope* that the interface gets frozen before it changes again.

Btw, could somebody provide clarification on the differences in behavior between the get/setCookieStringFromHttp and get/setCookieString methods of nsICookieService regarding the handling of 'httponly' cookies? The interfaces seem to predate the implementation of httponly cookie support in Firefox, and public documentation about the effect on the behavior of this interface is not findable. We need to make sure we are not inadvertantly exposing httponly cookies to partial-trust code. [Since this issue is somewhat tangential to the present bug, feel free to email me directly any relevant info. And if you wonder why I don't "just read the source code", it's complicated legally here... :-( ]

Thanks again for the cooperation.
(In reply to comment #10)
> However, it appears we don't have an alternative to nsICookieService presently.

agreed, you definitely want to use the methods on nsICookieService then. i'm not sure what we usually do in situations like this, but it seems to me that splitting out the critical methods into a frozen API (perhaps specific to embeddors), and leaving the rest of the interface unfrozen (e.g. nsICookieServiceInternal) might be the way to go. i'll look into this.

> So, at this point it seems we could only ship a patch with the new
> nsICookieService UUID and *hope* that the interface gets frozen before it
> changes again.

since you're using this API already, any change we make complicates things further, right? so i imagine we want to do this once and get it right.

> Btw, could somebody provide clarification on the differences in behavior
> between the get/setCookieStringFromHttp and get/setCookieString methods of
> nsICookieService regarding the handling of 'httponly' cookies?

get/setCookieStringFromHttp are called from networking, and handle data coming from HTTP headers; they allow cookies to be accepted and returned with the httponly flag. get/setCookieString handle the META HTTP-EQUIV tag and document.cookie, and will not accept or return cookies with the httponly flag. if you're dealing with HTTP Set-Cookie and Cookie headers, you want to be using get/setCookieStringFromHttp. let me know if you'd like any more clarification here...
Trying to make a patch for our released component... It appears that the FF v3 implementation of nsICookieService has further behavior change: No matter what URIs are passed to it, cookies are always treated as third-party. If access to third-party cookies is disabled, the interface seems to become useless. 

In our original use, we did something like this:
  pCookieService->SetCookieStringFromHttp(
     spUri, spTopLevelUri, nsnull, cookieData, nsnull, nsnull);
spUri is the URI for which to set the cookie; spTopLevelUri is what the user supposedly navigated to, which could be the same as spUri or something different, possibly referring to a different domain. Based on these parameters, the browser was able to correctly determine first- vs. third-party status of the cookie and apply the cookie handling policy from user preferences.

To get the original behavior, tried passing an nsIChannel with the LOAD_INITIAL_DOCUMENT_URI and LOAD_DOCUMENT_URI flags, but it doesn't help.

Any hints? Intentional change? Did we use the interface correctly initially?

Thanks.
Yes, there was apparently a change

FF2:
http://mxr.mozilla.org/mozilla1.8/source/netwerk/cookie/src/nsCookieService.cpp#687
FF3:
http://mxr.mozilla.org/mozilla/source/netwerk/cookie/src/nsCookieService.cpp#669

The aFirstURI argument is completely ignored in FF3, instead we try to get that value from the channel itself (see nsCookieService::CheckPrefs). If your channel doesn't have a docshell this is going to fail. See the 30-line comment at http://mxr.mozilla.org/mozilla/source/extensions/cookie/nsCookiePermission.cpp#425

So it looks like you were using the interface correctly for Firefox 2, and as there was trouble trusting the passed-in top-level URI the behavior changed in Firefox 3 even though that was not made clear in the interface itself.
nsIChannel, nsIRequest (transitivity), and nsIInterfaceRequestor are frozen:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIChannel.idl#60
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIInterfaceRequestor.idl#53

So, you should be able to forge an nsIChannel. Note that while nsIDocShell is not frozen, nsIDocShellTreeItem which is what you actually need is in a state of undeclared frozenness, bz would like to freeze it as we have a number of consumers who need it and it isn't something for which we'd want to make many changes.... 

This isn't a promise that it won't change, but the likelihood of it changing is considerably lower than nsIDocShell :), and if we were to change nsIDocShellTreeItem, there'd be a post to .platform or something.

http://mxr.mozilla.org/mozilla/source/docshell/base/nsIDocShellTreeItem.idl

Based on the code, it looks like you could do this:

class
MSGeckoChannel : nsIChannel, nsIInterfaceRequestor, nsIDocShellTreeItem
{
NS_DECL_ISUPPORTS NS_DECL_NSICHANNEL NS_DECL_NSIREQUEST NS_DECL_NSIINTERFACEREQUESTOR NS_DECL_NSIDOCSHELLTREEITEM NS_DECL_NSIDOCSHELLTREENODE

MSGeckoChannel(nsIURI *aURI): mURI(aURI);

private:
~MSGeckoChannel(){};
nsCOMPtr<nsIURI> mURI;
}

NS_IMPL_ISUPPORTS5(MSGeckoChannel, nsIChannel, nsIRequest, nsIInterfaceRequestor, nsIDocShellTreeItem, nsIDocShellTreeNode)

nsresult
MSGeckoChannel::GetNotificationCallbacks(nsIInterfaceRequestor **aResult) {
  *aResult = this;
  NS_ADDREF_THIS();
  return NS_OK;
}

nsresult
MSGeckoChannel::GetInterface(nsIID &aIID, void **result) {
  if (!aIID.Equals(NS_GET_IID(nsIDocShellTreeItem))
    return NS_ERROR_NOT_AVAILABLE;

  *result = this;
  NS_ADDREF_THIS();
  return NS_OK;
}

nsresult
MSGeckoChannel::GetSameTypeRootTreeItem(nsIDocShellTreeItem **aResult) {
  *aResult = this;
  NS_ADDREF_THIS();
  return NS_OK;
}

nsresult
MSGeckoChannel::GetItemType(PRUint32 **aType) {
  *aType = nsIDocShellTreeItem::typeContent;
  return NS_OK;
}

nsresult
MSGeckoChannel::GetLoadFlags(PRUint32 **aFlags) {
  *aFlags = nsIChannel::LOAD_DOCUMENT_URI;
  return NS_OK;
}

nsresult
MSGeckoChannel::GetURI(nsIURI **aResult) {
  NS_ADDREF(*aResult = mURI);
  return NS_OK;
}

obviously you'd need to add whichever other methods are needed to make the class compile.

  nsRefPtr<MSGeckoChannel> geckoChannel = new MSGeckoChannel(spUri);
  if (!spUri)
    return NS_ERROR_OUT_OF_MEMORY;
  pCookieService->SetCookieStringFromHttp(
     spUri, spTopLevelUri, nsnull, cookieData, nsnull, geckoChannel);

Should work, minus compiler complaints.
Attachment #355340 - Flags: review?(dwitte)
Comment on attachment 355340 [details] [diff] [review]
an explanation in slightly long and not particularly pleasant terms

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