James Zhang (Spreadtrum) Reporter
	Description • 11 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=970008#c24
https://bugzilla.mozilla.org/show_bug.cgi?id=970008#c30

We need minidump screen shot when monkey test.

	Andreas Gal :gal
	Comment 2 • 11 years ago

I am busy with my day job, probably not time to hack on this.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 3 • 11 years ago

to Vincent Liu since Vincent's working on it
1.3T+ to enable debugging for stability tests

	James Zhang (Spreadtrum) Reporter
	Comment 4 • 11 years ago

We wait for this patch.

	Vincent Liu[:vliu] Assignee
	Comment 5 • 11 years ago

My tarako build seems got corruption and currently try to clone it to look into this issue.

	Andreas Gal :gal
	Comment 6 • 11 years ago

Any progress here? please ask for help if you are stuck.

	Steven Yang [:styang]
	Comment 7 • 11 years ago

(In reply to Vincent Liu[:vliu] from comment #5)
> My tarako build seems got corruption and currently try to clone it to look
> into this issue.

Hi Vincent, could you update the progress on your side? thanks.

	Steven Yang [:styang]
	Comment 8 • 11 years ago

From Keven Kuo, Vincent Liu and Alan are working on it and will finish it by tomorrow.

	Vincent Liu[:vliu] Assignee
	Comment 9 • 11 years ago

Currently two files are reported when the crash happens. One is *.dmp file which is generated by google-breakpad. The file is generated when CrashGenerationServer::MakeMinidumpFilename() is called. It contains the backtrace info for crash. The other one is *.extra file which is generated by ContentParent::ActorDestroy() if the crash happens on content side. It contains some product and content info.
I took a little time to think about the suitable call flow following the above but it seems not to work out. Instead, I would have a hacks to let the function working on tarako at first.

	(not currently active) Ted Mielczarek
	Comment 10 • 11 years ago

Generally we put this functionality into the test runner and not the application itself. For example in the desktop Mochitest/Reftest harnesses, we have this code:
http://hg.mozilla.org/mozilla-central/annotate/082761b7bc54/build/automationutils.py#l494

	Vincent Liu[:vliu] Assignee
	Comment 11 • 11 years ago

I made a temp WIP for minidump screen shot. With this patch, minidump generated screenshot.raw in the path /data/b2g/mozilla/Crash Reports/pending/. Because I used fixed path and file name to generate this file, please make sure you don't keep this file in your device before you do any operation. There are some points still need to be improved

1. The generated file name can vary each time the crash happens. It's better following the rule the *.dmp or *.extra did.

2. Still consider the call flow whether it should follows  *.dmp or *.extra did. (if possible)

	Vincent Liu[:vliu] Assignee
	Comment 12 • 11 years ago

(In reply to Vincent Liu[:vliu] from comment #11)

> 1. The generated file name can vary each time the crash happens. It's better
> following the rule the *.dmp or *.extra did.
> 

WIP-v2 was attached for generating a flexiable dumped file name.

	Vincent Liu[:vliu] Assignee
	Comment 13 • 11 years ago

The patch was attached for review. After discuss with Graphic team, this patch is valid only for ICS platform. The reason is that after JB, it uses framebufferSurface (through Gralloc Hal) to get framebuffer.

Hi ehsan,
Would you please have a review for me? Thanks

	(no longer active)
	Comment 14 • 11 years ago

Comment on attachment 8394570 [details] [diff] [review]
bug-983022-fix.patch

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

Ted is a better reviewer for this code.

	Andreas Gal :gal
	Comment 15 • 11 years ago

We need a way to enable/disable this. This shouldn't be enabled on production devices.

	Andreas Gal :gal
	Comment 16 • 11 years ago

Comment on attachment 8394570 [details] [diff] [review]
bug-983022-fix.patch

Please allow this to be enabled/disabled via a prop.

	Andreas Gal :gal
	Comment 17 • 11 years ago

Comment on attachment 8394570 [details] [diff] [review]
bug-983022-fix.patch

Ted, please review this assuming we add a way to disable this by default.

	(not currently active) Ted Mielczarek
	Comment 18 • 11 years ago

Comment on attachment 8394570 [details] [diff] [review]
bug-983022-fix.patch

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

I still don't understand why we're baking this into the platform and not a test harness, but in the interest of expediency I'm not morally opposed to landing this.

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +80,5 @@
>  #include "nsIFile.h"
>  #include "prprf.h"
>  #include <map>
>  #include <vector>
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION ==  15

Might be nice to define something like SCREENSHOT_ENABLED so you don't have to repeat this complicated ifdef.

@@ +2059,5 @@
> +bool
> +WriteScreenShotToFile(const nsAString& id)
> +{
> +  ScopedClose fb(open("/dev/graphics/fb0", O_RDONLY));
> +  if (0 > fb.get()) {

nit: writing the comparisons this way doesn't match the rest of the file.

@@ +2085,5 @@
> +  sprintf(path, "%s/%s.raw", pendingDirectory, NS_ConvertUTF16toUTF8(id).get());
> +  ScopedClose fd(open(path, O_WRONLY | O_CREAT | O_TRUNC));
> +  if (0 > fd.get()) {
> +    close(fb);
> +    munmap(bits, fi.smem_len);    

nit: trailing whitespace

	Andreas Gal :gal
	Comment 19 • 11 years ago

How can we do this in the harness?

	Vincent Liu[:vliu] Assignee
	Comment 20 • 11 years ago

(In reply to Andreas Gal :gal from comment #16)
> Comment on attachment 8394570 [details] [diff] [review]
> bug-983022-fix.patch
> 
> Please allow this to be enabled/disabled via a prop.

Thanks for the good suggestion. I will add it in the next version of my patch.

	Vincent Liu[:vliu] Assignee
	Comment 21 • 11 years ago

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18)
> Comment on attachment 8394570 [details] [diff] [review]
> bug-983022-fix.patch
> 
> Review of attachment 8394570 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still don't understand why we're baking this into the platform and not a
> test harness, but in the interest of expediency I'm not morally opposed to
> landing this.
> 

Thanks the review for the patch. Following the suggestion from you, I will have a next version of patch. The situation we need this screenshot is when device ecoounter any unexpected crash, kerenl may sent signal to kill content process, plus triggering minidump to generate report. In such of case, Mochitest/Reftest might not aware of this. Please correct me if I got anything wrong. Or if you can have a good way or any suggestion, please feel free to let me know. I am glad to follow it.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 22 • 11 years ago

triage: minus, this will not block release. 
this is helpful for debugging stability bugs. please ask for approval to land in 1.3T

	Vincent Liu[:vliu] Assignee
	Comment 23 • 11 years ago

The patch was attached based on suggestions from last review.

1. Add environment variable MOZ_CRASHREPORTER_DUMP_FB to enable/disable via a prop.

2. Matched the comparison rule in the file.

3. Remove close() for the opend fb/fd based on bug 924729 Comment 10.

	Andreas Gal :gal
	Comment 24 • 11 years ago

Comment on attachment 8396946 [details] [diff] [review]
bug-983022-fix-v2.patch

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

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +2068,5 @@
> +    struct fb_fix_screeninfo fi;
> +    if (ioctl(fb.get(), FBIOGET_FSCREENINFO, &fi) < 0)
> +      return false;
> +
> +    void* bits = mmap(0, fi.smem_len, PROT_READ, MAP_SHARED,

this fits onto one line

@@ +2073,5 @@
> +                      fb.get(), 0);
> +    if (MAP_FAILED == bits)
> +      return false;
> +
> +    char path[128];

There is probably a proper constant for this. Please use that. MAX_PATH or so.

	Vincent Liu[:vliu] Assignee
	Comment 25 • 11 years ago

Based on review suggestion on Comment 24, the bug-983022-v3.patch was created.

	Andreas Gal :gal
	Comment 26 • 11 years ago

Comment on attachment 8399262 [details] [diff] [review]
bug-983022-v3.patch

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

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +2056,5 @@
>  bool
> +WriteScreenShotToFile(const nsAString& id)
> +{
> +  const char *envvar = PR_GetEnv("MOZ_CRASHREPORTER_DUMP_FB");
> +  if (envvar && *envvar) {

early out instead of long blocks, so

if (!envvar || !*envvar)
  return true;

@@ +2057,5 @@
> +WriteScreenShotToFile(const nsAString& id)
> +{
> +  const char *envvar = PR_GetEnv("MOZ_CRASHREPORTER_DUMP_FB");
> +  if (envvar && *envvar) {
> +    if (!pendingDirectory) return false;

please check the style of this file, return goes onto a new line

@@ +2063,5 @@
> +    ScopedClose fb(open("/dev/graphics/fb0", O_RDONLY));
> +    if (fb.get() < 0) return false;
> +
> +    struct fb_fix_screeninfo fi;
> +    if (ioctl(fb.get(), FBIOGET_FSCREENINFO, &fi) < 0) return false;

dito

@@ +2066,5 @@
> +    struct fb_fix_screeninfo fi;
> +    if (ioctl(fb.get(), FBIOGET_FSCREENINFO, &fi) < 0) return false;
> +
> +    void* bits = mmap(0, fi.smem_len, PROT_READ, MAP_SHARED,
> +                      fb.get(), 0);

this can go on one line

@@ +2067,5 @@
> +    if (ioctl(fb.get(), FBIOGET_FSCREENINFO, &fi) < 0) return false;
> +
> +    void* bits = mmap(0, fi.smem_len, PROT_READ, MAP_SHARED,
> +                      fb.get(), 0);
> +    if (MAP_FAILED == bits) return false;

dito

	Vincent Liu[:vliu] Assignee
	Comment 27 • 11 years ago

> early out instead of long blocks, so
> 
> if (!envvar || !*envvar)
>   return true;
> 

I think if the envvar or *envvar is null, it would be better returning false to warn that the *.raw file doesn't been written by some reason.

	Andreas Gal :gal
	Comment 28 • 11 years ago

Comment on attachment 8400501 [details] [diff] [review]
bug-983022-fix-v4.patch

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

No need to request review again. Just fix the nits and land it. Thanks!

::: toolkit/crashreporter/nsExceptionHandler.cpp
@@ +2059,5 @@
> +  const char *envvar = PR_GetEnv("MOZ_CRASHREPORTER_DUMP_FB");
> +  if (!envvar || !*envvar)
> +    return false;
> +
> +  if (!pendingDirectory)

This can be folded into the statement above.

@@ +2081,5 @@
> +  if (fd.get() < 0) {
> +    munmap(bits, fi.smem_len);
> +    return false;
> +  }
> +  write(fd.get(), bits, fi.smem_len);

Newline after the }

	Vincent Liu[:vliu] Assignee
	Comment 29 • 11 years ago

(In reply to Andreas Gal :gal from comment #28)
> Comment on attachment 8400501 [details] [diff] [review]
> bug-983022-fix-v4.patch
> 
> Review of attachment 8400501 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> No need to request review again. Just fix the nits and land it. Thanks!
> 
> ::: toolkit/crashreporter/nsExceptionHandler.cpp
> @@ +2059,5 @@
> > +  const char *envvar = PR_GetEnv("MOZ_CRASHREPORTER_DUMP_FB");
> > +  if (!envvar || !*envvar)
> > +    return false;
> > +
> > +  if (!pendingDirectory)
> 
> This can be folded into the statement above.
> 
> @@ +2081,5 @@
> > +  if (fd.get() < 0) {
> > +    munmap(bits, fi.smem_len);
> > +    return false;
> > +  }
> > +  write(fd.get(), bits, fi.smem_len);
> 
> Newline after the }

Thanks for the good suggestions and will modify them before patch landed.

	Vincent Liu[:vliu] Assignee
	Comment 30 • 11 years ago

The attached patch is proposed to land.

	Vincent Liu[:vliu] Assignee
	Comment 31 • 11 years ago

Hi Joe,

The patch is proposed to land to v1.3T branch. Do we need mark it as 1.3T+ to land it?

	Joe Cheng [:jcheng] (please needinfo)
	Comment 32 • 11 years ago

triage: before we have the approval 1.3T flag ready, i will 1.3T+ for getting this into tarako. thanks

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/234d50648ad1

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 11 years ago

Backed out for bustage on most platforms.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6531852947f

https://tbpl.mozilla.org/php/getParsedLog.php?id=37219900&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=37219918&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=37219920&tree=Mozilla-Inbound

	Vincent Liu[:vliu] Assignee
	Comment 35 • 11 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #34)
> Backed out for bustage on most platforms.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e6531852947f
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37219900&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37219918&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37219920&tree=Mozilla-
> Inbound

I am sorry that I didn't mark a=1.3T+ in the patch to point out it only land to v1.3T branch. I through it is enough to mark this bug as 1.3T+ in blocking-b2g. Can you please help to only land it into 1.3T branch? Thanks

I saw the wiki and know the land flow for v1.3T. I will notice it next time. 

https://wiki.mozilla.org/Release_Management/B2G_Landing#v1.3T

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/eef32863a798