I guess this is a long-standing problem, but I couldn't find another bug that covered this. I'm trying to come up with a minimal build, so I figured I'd try to fix this.

Since cookies are sort of an HTTP-specific extension, seems like the interface (if not the implementation) ought to go with HTTP. Anyone else buy that?

Comment on attachment 60406 [details] [diff] [review] move nsICookieService.idl to mozilla/protocol/http/public thanks for doing this crhis, r=morse

cc'ing shaver, because he made this work a while ago. My understanding was that cookies was supposed to be smart enough to hook itself into http, and intercept headers at will.. that way http wouldn't even need to know about nsICookieService

cookies are not http specific. e.g., file:// URLs can set cookies. in fact, any protocol which serves up an HTML document can set cookies (<meta HTTP-EQUIV="Set-Cookie" CONTENT="blah blah">). also, multipart/x-mixed-replace content can also set cookies.

how about creating another subdirectory in netwerk/ netwerk/cookie netwerk/cookie/public netwerk/cookie/src seems like this would be the natural place for the cookie database to live. the cookie UI code, however, should not be here, but should perhaps remain in extensions/cookie.

More bug gifting! darin, if you don't want to finish this, reassign back to me and I'll get to it after my sabbatical.

I'm on the case. (And I'm not taking any pansy sabbatical, either.)

Attaching revised patch based on waterson's original patch but incorporating darin's suggestion. Using directory mozilla/netwerk/base/public since that's where other outside interfaces have already migrated into (e.g., nsIPasswordManager.idl).

i would prefer to see cookies live in netwerk/cookies/public netwerk/cookies/src are you planning on moving the cookie service into necko as well? i believe it would be a good idea since many HTTP sites require the use of cookies. of course, the UI for cookies should remain outside of necko. is this the consensus of everyone? or are we only trying to solve _this_ bug by moving an interface?

No, I'm not planning on moving the cookie service implementation into necko. There was vehement objection to doing that when the cookie service was originally implemented, and I don't want to reopen that can of worms. Do you have strong objections to putting it into base/public? If not, I'd prefer putting it there. There alread are other external interfaces there as I mentioned. I didn't want to create a new directory just for the cookies/public, especially since there will not be a cookies/src.

need reviews. darin and alecf, please review.

Comment on attachment 65471 [details] [diff] [review] moving nsICookieService.idl to netwerk/base/public I'll defer to darin for the ultimate location of the files, but in any case r=alecf on the moving, wherever they may end up :) Don't forget the mac project files & build scripts - I don't see them in this patch

OK, i'm curious... who was objecting to having necko contain the cookie service/database? so far, i haven't heard any objections from other people i've mentioned this to. necko seems like the logical place for cookies since many HTTP sites require support for cookies. for example, many sites send a redirect w/ a Set-Cookie header and expect the cookie to be returned to the server in the redirected request. without cookie support on the client side, the result is typically an infinite redirection loop as the server keeps setting the cookie expecting to get it back on the next request. IMO necko should have some basic builtin support for cookies. otherwise, standalone consumers of necko will be forced to pull in the cookies extension for cookies support. unfortunately, this currently means pulling in the cookies UI and related dependencies. alternatively, consumers have to implement support for cookies on their own. neither solution is very good IMO for the future of necko. at the very least i would like to see nsICookieService.idl moved into necko. this is all that is really required in the mozilla 1.0 timeframe (i think). moving the service into necko can be done later. so, let's just move the interface into the right place from the start.

Hear! Hear! Putting cookie support into necko is what I wanted from the start, but it was rejected. I believe that valeski was one of the vocal opponents to this. And gagan I think was another. dp may recall others as well because I remember that he was at a meeting when this was discussed. So you won't hear any objections from me for doing so. So are you saying that all you are asking for now is that I use the netwerk/cookies/public directory as the home for the interface, then you will approve this patch. If so, consider it done (or do you want me to physically generate another patch?).

cc'ing neeti because she was working on cookies at the time that this decision was made, and she remember who was opposed to putting the cookie module into necko.

we made the decision to move cookies into extensions long before the current design/semantics of the directory structure fell into place. at the time, it was the right thing to do, now that things have matured a bit more, moving it into necko makes more sense.

glad to hear it :-) morse: yeah, no need for another patch. moving nsICookieService into netwerk/cookie/public should do the trick for now.

in other words, sr=darin on the move of nsICookieService.idl to netwerk/cookie/public

I forget the historical details on why it was decided to keep the module external. But I suspect valeski is right and additionally I think when we were attempting to build the 'minimal' build the definition may not have included cookies. Also IIRC there wasn't a way to isolate cookies basic functionality from the external manager to control access to them. This may have been cleaned up now in which case it would be nice to have the cookie service back in necko. Though clearly the managing of the cookies (filters, UI, etc.) part should still live as an extension.

Comment on attachment 65471 [details] [diff] [review] moving nsICookieService.idl to netwerk/base/public Recording darin's sr= with the patch manager. Any Mac build-file changes needed? /be

> Any Mac build-file changes needed? Possibly. I was about to look into that today and I'll probably need to get a mac buddy. Jag, are you available to help out for the mac part?

Comment on attachment 66535 [details] [diff] [review] moving nsICookieService.idl to netwerk/cookie/public r/sr = alecf,darin

Hmm. The 1/25 patch (putting nsICookieService.idl into netwerk/cookie/pubic) doesn't seem to work -- browser gets stuck in profile manager. The 1/17 patch (putting it into netwerk/base/public) works fine. I'm investigating.

There are definitely some serious problems with this patch. Turns out the 1/17 patch didn't work either -- it only seemed to work because I didn't do a clobber build. If you do a clobber build with either patch you get several debug assertions and then finally you get the following crash. xptiInterfaceEntry::GetIIDForParamNoAlloc(unsigned short 12, const nsXPTParamInfo * 0x00128234, nsID * 0x0012836c) line 475 + 6 bytes xptiInterfaceInfo::GetIIDForParamNoAlloc(xptiInterfaceInfo * const 0x0384d440, unsigned short 12, const nsXPTParamInfo * 0x00128234, nsID * 0x0012836c) line 725 + 46 bytes GetInterfaceTypeFromParam(XPCCallContext & {...}, nsIInterfaceInfo * 0x0384d440, const nsXPTMethodInfo * 0x031f99ac, const nsXPTParamInfo & {...}, unsigned short 12, unsigned char 0, const nsXPTType & {...}, nsXPTCVariant * 0x001282e8, nsID * 0x0012836c) line 1550 + 25 bytes XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode CALL_GETTER) line 2057 + 75 bytes XPCWrappedNative::GetAttribute(XPCCallContext & {...}) line 1818 + 14 bytes XPC_WN_GetterSetter(JSContext * 0x01ef3350, JSObject * 0x03205980, unsigned int 0, long * 0x031edc34, long * 0x001285d8) line 1298 + 12 bytes js_Invoke(JSContext * 0x01ef3350, unsigned int 0, unsigned int 2) line 832 + 23 bytes js_InternalInvoke(JSContext * 0x01ef3350, JSObject * 0x03205980, long 52451808, unsigned int 0, unsigned int 0, long * 0x00000000, long * 0x00129388) line 924 + 20 bytes js_GetProperty(JSContext * 0x01ef3350, JSObject * 0x03205980, long 47806512, long * 0x00129388) line 2447 + 45 bytes js_Interpret(JSContext * 0x01ef3350, long * 0x00129538) line 2631 + 1998 bytes js_Invoke(JSContext * 0x01ef3350, unsigned int 0, unsigned int 2) line 849 + 13 bytes js_InternalInvoke(JSContext * 0x01ef3350, JSObject * 0x03204dc0, long 52450456, unsigned int 0, unsigned int 0, long * 0x00000000, long * 0x0012a2a0) line 924 + 20 bytes js_GetProperty(JSContext * 0x01ef3350, JSObject * 0x03204dc0, long 47806512, long * 0x0012a2a0) line 2447 + 45 bytes js_Interpret(JSContext * 0x01ef3350, long * 0x0012a450) line 2631 + 1998 bytes js_Invoke(JSContext * 0x01ef3350, unsigned int 1, unsigned int 2) line 849 + 13 bytes js_InternalInvoke(JSContext * 0x01ef3350, JSObject * 0x03204b48, long 52450816, unsigned int 0, unsigned int 1, long * 0x0012a6a8, long * 0x0012a578) line 924 + 20 bytes JS_CallFunctionValue(JSContext * 0x01ef3350, JSObject * 0x03204b48, long 52450816, unsigned int 1, long * 0x0012a6a8, long * 0x0012a578) line 3405 + 31 bytes nsJSContext::CallEventHandler(nsJSContext * const 0x01ef3500, void * 0x03204b48, void * 0x03205600, unsigned int 1, void * 0x0012a6a8, int * 0x0012a6ac, int 0) line 1016 + 33 bytes nsJSEventListener::HandleEvent(nsJSEventListener * const 0x036bf300, nsIDOMEvent * 0x03845498) line 180 + 77 bytes nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x036bf670, nsIDOMEvent * 0x03845498, nsIDOMEventTarget * 0x0364b238, unsigned int 8, unsigned int 2) line 1205 + 20 bytes nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x036bf350, nsIPresContext * 0x01f5f570, nsEvent * 0x03845110, nsIDOMEvent * * 0x0012b4fc, nsIDOMEventTarget * 0x0364b238, unsigned int 2, nsEventStatus * 0x0012b4c8) line 1806 + 36 bytes nsXULElement::HandleDOMEvent(nsXULElement * const 0x0364b230, nsIPresContext * 0x01f5f570, nsEvent * 0x03845110, nsIDOMEvent * * 0x0012b4fc, unsigned int 2, nsEventStatus * 0x0012b4c8) line 3375 nsXULElement::HandleDOMEvent(nsXULElement * const 0x0365c7d0, nsIPresContext * 0x01f5f570, nsEvent * 0x03845110, nsIDOMEvent * * 0x0012b4fc, unsigned int 1, nsEventStatus * 0x0012b4c8) line 3392 + 50 bytes nsEventStateManager::DispatchNewEvent(nsEventStateManager * const 0x0312f178, nsISupports * 0x0365c7d0, nsIDOMEvent * 0x03845498, int * 0x0012b6dc) line 3831 + 53 bytes nsXULElement::DispatchEvent(nsXULElement * const 0x0365c7d8, nsIDOMEvent * 0x03845498, int * 0x0012b6dc) line 1845 + 38 bytes XPTC_InvokeByIndex(nsISupports * 0x0365c7d8, unsigned int 5, unsigned int 2, nsXPTCVariant * 0x0012b6cc) line 106 XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode CALL_METHOD) line 1998 + 42 bytes XPC_WN_CallMethod(JSContext * 0x01ef3350, JSObject * 0x03204b98, unsigned int 1, long * 0x031edb30, long * 0x0012b9a8) line 1266 + 14 bytes js_Invoke(JSContext * 0x01ef3350, unsigned int 1, unsigned int 0) line 832 + 23 bytes js_Interpret(JSContext * 0x01ef3350, long * 0x0012c798) line 2800 + 15 bytes js_Invoke(JSContext * 0x01ef3350, unsigned int 1, unsigned int 2) line 849 + 13 bytes js_InternalInvoke(JSContext * 0x01ef3350, JSObject * 0x03204b98, long 52449792, unsigned int 0, unsigned int 1, long * 0x0012d588, long * 0x0012d588) line 924 + 20 bytes js_SetProperty(JSContext * 0x01ef3350, JSObject * 0x03204b98, long 48172368, long * 0x0012d588) line 2599 + 47 bytes js_Interpret(JSContext * 0x01ef3350, long * 0x0012d738) line 2642 + 1939 bytes js_Invoke(JSContext * 0x01ef3350, unsigned int 1, unsigned int 2) line 849 + 13 bytes js_InternalInvoke(JSContext * 0x01ef3350, JSObject * 0x03204b88, long 52449440, unsigned int 0, unsigned int 1, long * 0x0012e528, long * 0x0012e528) line 924 + 20 bytes js_SetProperty(JSContext * 0x01ef3350, JSObject * 0x03204b88, long 47696032, long * 0x0012e528) line 2599 + 47 bytes js_Interpret(JSContext * 0x01ef3350, long * 0x0012e6d8) line 2642 + 1939 bytes js_Invoke(JSContext * 0x01ef3350, unsigned int 1, unsigned int 2) line 849 + 13 bytes js_InternalInvoke(JSContext * 0x01ef3350, JSObject * 0x03204b88, long 52450616, unsigned int 0, unsigned int 1, long * 0x0012e930, long * 0x0012e800) line 924 + 20 bytes JS_CallFunctionValue(JSContext * 0x01ef3350, JSObject * 0x03204b88, long 52450616, unsigned int 1, long * 0x0012e930, long * 0x0012e800) line 3405 + 31 bytes nsJSContext::CallEventHandler(nsJSContext * const 0x01ef3500, void * 0x03204b88, void * 0x03205538, unsigned int 1, void * 0x0012e930, int * 0x0012e934, int 0) line 1016 + 33 bytes nsJSEventListener::HandleEvent(nsJSEventListener * const 0x038427c0, nsIDOMEvent * 0x03841098) line 180 + 77 bytes nsXBLPrototypeHandler::ExecuteHandler(nsXBLPrototypeHandler * const 0x038129b0, nsIDOMEventReceiver * 0x03660b98, nsIDOMEvent * 0x03841098) line 442 nsXBLPrototypeHandler::BindingAttached(nsXBLPrototypeHandler * const 0x038129b0, nsIDOMEventReceiver * 0x03660b98) line 490 nsXBLPrototypeBinding::BindingAttached(nsXBLPrototypeBinding * const 0x038118c0, nsIDOMEventReceiver * 0x03660b98) line 439 + 33 bytes nsXBLBinding::ExecuteAttachedHandler(nsXBLBinding * const 0x03829d50) line 1039 nsBindingManager::ProcessAttachedQueue(nsBindingManager * const 0x01ef7220) line 912 nsCSSFrameConstructor::ContentInserted(nsCSSFrameConstructor * const 0x01f64780, nsIPresContext * 0x01f5f570, nsIContent * 0x02f607c0, nsIContent * 0x0364b230, int -1, nsILayoutHistoryState * 0x00000000, int 0) line 8738 StyleSetImpl::ContentInserted(StyleSetImpl * const 0x01f64a60, nsIPresContext * 0x01f5f570, nsIContent * 0x02f607c0, nsIContent * 0x0364b230, int -1) line 1446 PresShell::ContentInserted(PresShell * const 0x01f64348, nsIDocument * 0x01eff3a0, nsIContent * 0x02f607c0, nsIContent * 0x0364b230, int -1) line 5155 + 53 bytes nsXBLResourceLoader::NotifyBoundElements() line 277 nsXBLResourceLoader::StyleSheetLoaded(nsXBLResourceLoader * const 0x036ee750, nsICSSStyleSheet * 0x036e7e40, int 1) line 207 CSSLoaderImpl::InsertSheetInDoc(nsICSSStyleSheet * 0x036e7e40, int 2, nsIContent * 0x00000000, int 1, nsICSSLoaderObserver * 0x036ee750) line 1202 InsertPendingSheet(void * 0x0381a6d0, void * 0x036dbbd0) line 763 nsVoidArray::EnumerateForwards(int (void *, void *)* 0x02130370 InsertPendingSheet(void *, void *), void * 0x036dbbd0) line 660 + 21 bytes CSSLoaderImpl::Cleanup(URLKey & {...}, SheetLoadData * 0x038195a0) line 827 CSSLoaderImpl::SheetComplete(nsICSSStyleSheet * 0x00000000, SheetLoadData * 0x038195a0) line 920 CSSLoaderImpl::ParseSheet(nsIUnicharInputStream * 0x036e2f40, SheetLoadData * 0x038195a0, int & 1, nsICSSStyleSheet * & 0x036e7e40) line 955 CSSLoaderImpl::DidLoadStyle(nsIStreamLoader * 0x03819420, nsString * 0x036e4fa0 {"/* * The contents of this file are subject to the Netscape Public * License Version 1.1 (the "License"); you may not use t"}, SheetLoadData * 0x038195a0, unsigned int 0) line 990 + 27 bytes SheetLoadData::OnStreamComplete(SheetLoadData * const 0x038195a0, nsIStreamLoader * 0x03819420, nsISupports * 0x00000000, unsigned int 0, unsigned int 3826, const char * 0x032210c8) line 747 nsStreamLoader::OnStopRequest(nsStreamLoader * const 0x03819424, nsIRequest * 0x0381af80, nsISupports * 0x00000000, unsigned int 0) line 163 nsJARChannel::OnStopRequest(nsJARChannel * const 0x0381af84, nsIRequest * 0x0381aed4, nsISupports * 0x00000000, unsigned int 0) line 611 + 49 bytes nsOnStopRequestEvent::HandleEvent() line 213 nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x036642e4) line 116 PL_HandleEvent(PLEvent * 0x036642e4) line 590 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x00a50ae0) line 520 + 9 bytes _md_EventReceiverProc(HWND__ * 0x32fd003c, unsigned int 49577, unsigned int 0, long 10816224) line 1071 + 9 bytes USER32! 77e71268() 00a50ae0()

I don't see an XPIDL_MODULE line in the new public/cookie makefiles, which leads me to wonder if you're not just clobbering the main necko typelib files. (And does windows clobber clear out old typelibs?) Or did this used to work, when the patch was first attached, reviewed and super-reviewed? Should we be looking for interactions with more recent checkins?

No, it probably never worked. I only thought it did because I didn't do a clobber build at the time.

steve, the difference between clobbering and not clobbering in the world of XPCOM is crucial :-) (as we're seeing w/ this change). XPCOM registration binds contract IDs to implementations dynamically (totally independent of compile time and gmake/nmake depend capability), at runtime (registration time at startup for most/many). So, when you're playing w/ XPCOM components/modules (moving their location, changing who implements a specific contract ID, or what contract ID a specific implementation impls), clobber builds become essential. with a more granular understanding of what's being changed, where, and when, one can do things like blow away the component.reg file and restart, but, clobbering is safest.

Jud, I'm well aware of that. I'm not defending the fact that I didn't clobber build originally after changing the interface. That was definitely an error on my part. I'm just saying that that explains why I originally thought that the 1-17 patch worked whereas in fact it probably never did.

Still haven't figured out why the patch doesn't work. But I do know what is causing the crash. Apparently the patch is causing the cookie module to not be initialized. Then in xptiInterfaceInfo.cpp::GetEntryForParam there is the following code: NS_ASSERTION(theEntry, "bad state"); *entry = theEntry; return NS_OK; So even though we know that theEntry is incorrect (null), we go ahead and assign it to entry and return an NS_OK status. The caller, GetIIDForParamNoAlloc, immediately dereferences entry and get's a crash. The fix is to modify the above to read NS_ASSERTION(theEntry, "bad state"); if (!theEntry) { return NS_ERROR_FAILURE; } *entry = theEntry; return NS_OK; I just opened a separate bug report on this problem. It's bug 122463

The problem is that the makefiles lack XPIDL_MODULE lines. These modules under necko are merged into one so we have multiple makefiles that claim their module is named necko. This is OK for the C++ code because we build static libs and link them into a dll. But for xpt files we build a xpt file in each directory and use the declared module name as the name of the file. This means that you build a necko.xpt file here and someone else builds one elsewhere and then one file overwrite the other in the components directory and some info is lost at runtime. This is why XPIDL_MODULE exists. It tells the build system to use the given name as the name of the generated xpt file rather than using the module name. If you add the following to the makefiles for win and Unix it should be OK: XPIDL_MODULE = necko_cookie See for examples: http://lxr.mozilla.org/seamonkey/source/netwerk/cache/public/Makefile.in http://lxr.mozilla.org/seamonkey/source/netwerk/cache/public/Makefile.win

YES, YES, YES!!! That fixed it. Thanks, John. I realize now that that's what shaver was saying in comment #27. I didn't understand his comment then because this line was not needed in the interface before it was moved. But with your explanation, it's now clear why that was so. Attaching new patch in a few minutes.

Comment on attachment 67024 [details] [diff] [review] adding XPIDL_MODULE sr=darin

darin, alecf, please review. Thanks. jag, please modify the mac project files appropriately. Thanks.

Comment on attachment 67024 [details] [diff] [review] adding XPIDL_MODULE sr=alecf

Tested this on Mac and Linux. I had to change the DEPTH in netwerk/cookies/public/Makefile.in to get it working.

note: mac projects are now xml based so you don't need a "mac buddy" to do build system mods (though, verification is always a good thing :-) ) http://www.mozilla.org/build/mac-build-system.html#How_to_add_a_new_project_to_the_build

Comment on attachment 67078 [details] [diff] [review] Add Mac changes sr=sfraser for mac build changes

checked in.