Closed Bug 113519 Opened 23 years ago Closed 23 years ago

cannot build with cookie extension disabled

Categories

(Core :: Networking: Cookies, defect, P3)

Product:
Core
Component:
Networking: Cookies
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: waterson, Assigned: shaver)

References

Details

Attachments

(1 file, 4 obsolete files)

Add Mac changes
(deleted), patch
sfraser_bugs
: superreview+
Details | Diff | Splinter Review 
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.
Blocks: 18352
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.9.7
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?
Keywords: patch
Comment on attachment 60406 [details] [diff] [review]
move nsICookieService.idl to mozilla/protocol/http/public

thanks for doing this crhis,

r=morse
Attachment #60406 - Flags: review+
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.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
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.
Assignee: waterson → darin
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.8 → ---
I'm on the case.  (And I'm not taking any pansy sabbatical, either.)
Assignee: darin → shaver
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).
Attachment #60406 - Attachment is obsolete: true
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
Attachment #65471 - Flags: review+
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
Attachment #65471 - Flags: superreview+
> 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
Attachment #66535 - Flags: superreview+
Attachment #66535 - Flags: review+
Attachment #65471 - Attachment is obsolete: true
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.
Attached patch adding XPIDL_MODULE (obsolete) (deleted) — — Splinter Review 

        Attachment #66535 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 67024 [details] [diff] [review]
adding XPIDL_MODULE

sr=darin

        Attachment #67024 -
        Flags: superreview+

    
    

    
  
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

        Attachment #67024 -
        Flags: review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Add Mac changes (deleted)
        — 
          — Splinter Review
      

    


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

        Attachment #67024 -
        Attachment is obsolete: true

    
    

    
  
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

        Attachment #67078 -
        Flags: superreview+

    
    

    
  
checked in.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: