1377923 - Large volume of warnings on nightly - WARNING: 'obs', file ... image/RasterImage.cpp, line 1402 (VectorImage.cpp / ImageFactory.cpp)

Status: RESOLVED DUPLICATE of bug 1377920

(Core :: ImageLib, defect)

Description

[Alex Gaynor [:Alex_Gaynor]]

STR:

1) Use debug build from central (a3b192dc8344)
2) Browse to basically any website (e.g. google.com)
3) Observe the parade of:

[Child 47418] WARNING: 'obs', file /Users/agaynor/projects/mozilla-central/image/RasterImage.cpp, line 1402

They spew out at several per second for me.

(Hopefully this is the correct component)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I bet this code really wanted to warn if the observer service _wasn't_ found...

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Just noticed this same code pattern occurs at |image/VectorImage.cpp| line 1032.

Comment 3

[Johann Hofmann [:johannh]]

Argh, sorry. Technically I'm on PTO, but I can see this is really annoying. I'll attach a patch. I'll rely on try to let me know if this compiles.

Comment 4

[Johann Hofmann [:johannh]]

Attachment: Bug 1377923 - Don't check if observer service is found in image loading/showing reporting code.

Review commit: https://reviewboard.mozilla.org/r/154114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/154114/

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 8883187 [details]
Bug 1377923 - Don't check if observer service is found in image loading/showing reporting code.

https://reviewboard.mozilla.org/r/154114/#review159292

Doh! I should have caught that in review.

::: image/ImageFactory.cpp:99
(Diff revision 1)
>  
>  #ifdef DEBUG
>    // Record the image load for startup performance testing.
>    if (NS_IsMainThread()) {
>      nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> -    if (NS_WARN_IF(obs)) {
> +    if (!NS_WARN_IF(!obs)) {

The double inversion punctuated by a warning is pretty awkward. Given that this is debug only code and that we know that GetObserverService() will not return null at any time we could be creating images (it needs to be one of the first things to start up, and last to shut down), I don't think we need to null check at all. Either MOZ_ASSERT instead or else do nothing and allow us to crash doing a null-deref a couple lines down. Either option would be much more visible than a warning, and in this case that would be a good thing since we'd really like to know why on earth the observer service isn't available here.

The same change would apply at the other places touched by this patch.

Comment 7

[Jorg K (CEST = GMT+2)]

Adding the actual warning to the summary to avoid further duplicates.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8883187 [details]
Bug 1377923 - Don't check if observer service is found in image loading/showing reporting code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/154114/diff/1-2/

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 8883187 [details]
Bug 1377923 - Don't check if observer service is found in image loading/showing reporting code.

https://reviewboard.mozilla.org/r/154114/#review159502

Comment 10

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7e5638fc75cc -d 62a566f7ab69: rebasing 405284:7e5638fc75cc "Bug 1377923 - Don't check if observer service is found in image loading/showing reporting code. r=jwatt" (tip)
merging image/ImageFactory.cpp
merging image/RasterImage.cpp
merging image/VectorImage.cpp
warning: conflicts while merging image/ImageFactory.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging image/RasterImage.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging image/VectorImage.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)