224339 - Image blocker depends on MailNews

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect, P2)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

Code in extensions/cookie/nsImgManager.cpp relies on the presence of default
preferences from mailnews.js in order to function without crashing. Not all
applications that wish to use image blocking include Mail/News (e.g. Firebird). 

Patch in a second...

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Blocks Firebird Installer. 

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch to fix.

Only build mail pref checking code if we're also building mail.

Comment 3

[Darin Fisher]

isn't the img manager included in the GRE?  wouldn't this be yet another place
where the GRE is getting forked between different applications?

btw, what is the crash?  can we just work around the crash?  img manager should
be made to be tolerant of preferences not being defined in any case whether
mailnews is compiled or not.

Comment 4

[[e-mail]]

um, huh? are you saying nsPrefBranch::AddObserver crashes if the specified pref
doesn't exist?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Imo that code in nsImgManager should in fact get split out into two different
classes, one part of core, maybe, one only part of tbird.  The core should know
nothing of the existence of either mailnews or firebird.

That said, I too would be interested to know why this crashes without that patch.

Comment 6

[[e-mail]]

Comment on attachment 134590 [details] [diff] [review]
patch to fix. 

so i talked to ben and i don't think there's a crasher here... at most we might
want to fork some portions of nsImgManager to reduce codesize for firebird.
this patch as it stands won't work because it leaves the mailnews boolpref
member undefined  #ifndef MOZ_MAIL_NEWS.

(forking this file is just fine; it's an app-specific conpol impl and won't be
part of the GRE).

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: better patch

Catch all instances of mBlockInMailNewsPref and wrap in a Mail specific ifdef. 


I'm not sure what was causing the crash either but that's gone now, it's just
an incredibly annoying assertion now which this patch fixes.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

-> me. 

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

What assertion?   Prefs are designed to work indepent of whether the prefs
actually exist or not.  The methods should return a failure code if the prefs
you are trying to play with don't exist.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Hmm?  So if I register an observer for a pref that's not set.... I'll get a
failure?  What if someone later sets it?

Comment 11

[Darin Fisher]

>Hmm?  So if I register an observer for a pref that's not set.... I'll get a
>failure?  What if someone later sets it?

no, you can register observers for non-existant prefs.  a lot of components
depend on that functionality.


as for this patch... why not do away with the assertion?  it seems like a bogus
assertion to me.  this module should be made to function correctly with no prefs
explicitly set in the pref service.  there should just be reasonable default
values for all prefs hardcoded in the service itself that can be used if no
prefs are explicitly set.  ifdefs make sense if you are trying to reduce
codesize, but... in this case we really aren't talking about much code :-/

adding ifdefs increases the chances that someone will regress the build (because
they may make a change that breaks when MOZ_MAIL_NEWS is not defined), so i
think it's worth avoiding unless it gives us a decent win of some sort.  right?

Comment 12

[Christopher Aillon (sabbatical, not receiving bugmail)]

Sorry, I didn't look at the patch to realize there were observers being added. 
What I meant though was that prefs don't assert (did ben maybe mean warn?), so I
assume that asserts are coming from elsewhere.  If #ifdef'ing causes the asserts
to go away, then maybe the assert is wrong.

Comment 13

[[e-mail]]

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

as darin and caillon said, there's little codesize win to be had here, so it'd
be better to just fix the source of the assertion (which is definitely bogus if
the pref is optional, as is the case here).

Comment 14

[[e-mail]]

Attachment: fix assertion + cleanup

this removes the bogus assertion completely, by way of shamelessly stealing
darin's lean-and-mean pref code from nsCookiePermission.

it also cleans up some cruft that was floating around...

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 135168 [details] [diff] [review]
fix assertion + cleanup

>+#define PREF_CHANGED(_P) (!aPref || !strcmp(aPref, _P))


Ewww.  #defines which rely on pre-existing symbols are lame.  Fix your #define
or use an inline method.


> NS_IMETHODIMP
> nsImgManager::Observe(nsISupports *aSubject,
>                       const char *aTopic,
>                       const PRUnichar *aData)
> {
...
>+  nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(aSubject);
>+  if (prefBranch)
>+    PrefChanged(prefBranch, NS_LossyConvertUTF16toASCII(aData).get());


You could probably lose the null check in favor of an assert.  The subject
should always QI to a prefbranch.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 135168 [details] [diff] [review]
fix assertion + cleanup

>You could probably lose the null check in favor of an assert.  The subject
>should always QI to a prefbranch.

... as long as your topic is NS_PREFBRANCH_PREFCHANGED_TOPIC_ID (you might also
want to assert that you get a known topic)

Comment 17

[[e-mail]]

Attachment: fix assertion + cleanup 2

addresses caillon's comments, and ports the same nitfixes to nsCookiePermission
too.

Comment 18

[timeless]

don't lose the if check. someday someone using js will have fun and pass some
other object.

Comment 19

[[e-mail]]

>don't lose the if check. someday someone using js will have fun and pass some
>other object.

then they will get to know about it, quickly and loudly :)

Comment 20

[[e-mail]]

actually, so i'll add back the nullchecks on the QI in Observe(), and also keep
the topic assertions. safer that way ;)

Comment 21

[Darin Fisher]

Comment on attachment 135170 [details] [diff] [review]
fix assertion + cleanup 2

>Index: extensions/cookie/nsCookiePermission.cpp

>+#define PREF_CHANGED(aPref, aChangedPref) (!aChangedPref || !strcmp(aChangedPref, aPref))

>-  if (PREF_CHANGED(kCookiesAskPermission) &&
>+  if (PREF_CHANGED(kCookiesAskPermission, pref) &&

bah!  why bother with this?  the existing code is plenty clear.  necko uses
this pattern already in places.  i don't see the problem using it here as
well.  i would totally agree if the macro were defined at the top of the
source file or in some header file, but it is defined right where it is
used so the reader can easily see what is going on.  but, whatever ;-)


> nsImgManager::nsImgManager()
>+ : mBehaviorPref(kImageBehaviorPrefDefault)
>+ , mBlockerPref(kImageBlockerPrefDefault)
>+ , mWarningPref(kImageWarningPrefDefault)
>+ , mBlockInMailNewsPref(kImageBlockImageInMailNewsPrefDefault)
> {

nit: indentation of the initializer list seems a bit unusual.  seems like
two blank spaces would be better.

sr=darin

Comment 22

[[e-mail]]

>bah!  why bother with this?  the existing code is plenty clear.

well, my reviewer asked for it - but since you commented, i'll change it back ;)

Comment 23

[[e-mail]]

done.

Comment 24

[Michiel van Leeuwen (email: mvl+moz@)]

*** Bug 199393 has been marked as a duplicate of this bug. ***