703912 - window.dump broken in debug mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Christian Holler (:decoder)]

This bug was filed from the Socorro interface and is 
report bp-3875dc99-c6d8-4909-8c87-8eadb2111119 .
============================================================= 

I just got the crash above on my debug build of Fennec and quickly inspected the code. I found this in dom/base/nsGlobalWindow.cpp:

      if (cstr) {
    #ifdef ANDROID
        __android_log_print(ANDROID_LOG_INFO, "Gecko", cstr);
    #endif
        FILE *fp = gDumpFile ? gDumpFile : stdout;
        fputs(cstr, fp);
        fflush(fp);
        nsMemory::Free(cstr);
      }
     
    #if defined(ANDROID) && defined(DEBUG)
      __android_log_print(ANDROID_LOG_INFO, "GeckoDump", "%s", cstr);
    #endif 


It seems like the second call to __android_log_print shouldn't be there, it uses cstr after free and without a null check.

Comment 1

[Christian Holler (:decoder)]

I forgot to mention that even the first log statement is wrong, as it misses the format string (and therefore cstr is the format string). Try something like "dump('%s %s %s') to get a crash. It should be done as in the second log call.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

:decoder, is this in native fennec or Xul fennec in terms of the debug version?

Comment 3

[Christian Holler (:decoder)]

This is native fennec (birch rev e056708c00aa).

Comment 4

[Christian Holler (:decoder)]

Attachment: Patch

The attached (trivial) patch just removes the second print call entirely and fixes the format string issue with the first one.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 576169 [details] [diff] [review]
Patch

Review of attachment 576169 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those two changes

::: dom/base/nsGlobalWindow.cpp
@@ +4572,5 @@
>  #endif
>  
>    if (cstr) {
>  #ifdef ANDROID
> +    __android_log_print(ANDROID_LOG_INFO, "Gecko", "%s", cstr);

I think it would be better if this were __android_log_write(ANDROID_LOG_INFO, "Gecko", cstr);

Also, "GeckoDump" might be a better tag than "Gecko"

Comment 6

[Christian Holler (:decoder)]

Attachment: Updated patch

Updated patch according to review comments. This needs to be checked in by someone :)

Comment 7

[Christian Holler (:decoder)]

Attachment: Updated patch

Rebased patch for mozilla-central and removed last patch hunk (code was already removed by someone else by now).

Comment 8

[Christian Holler (:decoder)]

Comment on attachment 591879 [details] [diff] [review]
Updated patch

r+ taken from previous patch.

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7d18fbe8c8

Should this be nominated for Aurora?

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ff7d18fbe8c8