199108 - Changes to cookie prefs need restart to take effect

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Christian Eyrich]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de-DE; rv:1.4a) Gecko/2003032416
Build Identifier: Mozilla/5.0 (Windows; U; Windows 98; de-DE; rv:1.4a) Gecko/2003032416

After making changes to cookie behaviour (Enable cookies, disable cookies, ask
before storing a.s.o) in ui I need to leave the browser and start it again to
take the changes effect.

E.g. changing from "Enable all cookies" to "Disable cookies" and leaving prefs
via ok changes nothing: cookies will be set further on.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Christian Eyrich]

Also occurs in current CVS build.

Comment 2

[[e-mail]]

->me, confirming

AddObserver is failing in nsCookiePrefObserver's ctor, because the latter isn't
an xpcom object and weakref can't QI to it.

two options:
a) merge nsCookiePrefObserver into the cookieservice (which involves some messy
hackery, since we need to pass |this| into nsCookies.cpp for now). this would've
been all taken care of nicely when we merge them all together, but that's not
happening for 1.4a, so this is a stopgap measure.

b) make it an xpcom interface so weakrefs work.

i've taken the former approach, fix is done. i'll run it by darin then post for
r/sr.

Comment 3

[[e-mail]]

Attachment: move prefobserver into cookieservice

darin: sorry for the size of the patch :/

it's mostly code movement, the only interesting bit is the ctor of
nsCookieService, where some |if| conditions changed. the rest is just code
movement and |this| hackery.

Comment 4

[[e-mail]]

Attachment: better patch

ok, this one follows darin's suggestion and is much cleaner.

all we had to do was call NS_ADDREF and NS_RELEASE properly, to avoid crashes
when the observer service tries to deal with weak refs.

Comment 5

[Darin Fisher]

Comment on attachment 118498 [details] [diff] [review]
better patch

yup, looks good. sr=darin

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 118498 [details] [diff] [review]
better patch

If I understand correctly, nsCookiePrefObserver::Init was already declared in
the header, but there was not yet any implementation? Since you are not adding
a header entry for that new function.

You are removing checks for mPrefBranch.
I have no knowledge whether it is safe to remove those checks, and you are not
explaining it in your comments.

If you are confident this is ok, r=kaie

Comment 7

[Kai Engert [:KaiE:]]

Oh, now I see, it's safe to no longer check mPrefBranch, since the Init function
won't succeed if mPrefBranch could not get created. So the patch makes sense.

Comment 8

[[e-mail]]

thanks kaie!

to address your two concerns:

1) yeah, the prototypes were there already; I forgot to clean them up at some
earlier stage, so I removed the cruft there.

2) sorry for not commenting about the checks... they're not necessary, since
nsCookiePrefObserver::Init will fail if it can't get mPrefBranch, so there's not
much point in checking it later on in life. and we still want to read the prefs
if ->AddObserver failed, so I moved that outside the |if|.

great, thanks again, ready for checkin!

Comment 9

[Darin Fisher]

fixed-on-trunk