[tarako]need minidump screen shot

RESOLVED FIXED

Status

()

defect
--
major
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: james.zhang, Assigned: vliu)

Tracking

28 Branch
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3T+, b2g-v1.3 unaffected, b2g-v1.3T fixed, b2g-v1.4 wontfix, b2g-v2.0 wontfix)

Details

(Whiteboard: [tarako_only])

Attachments

(1 attachment, 8 obsolete attachments)

blocking-b2g: --- → 1.3T?
Flags: needinfo?(ttsai)
Flags: needinfo?(styang)
Flags: needinfo?(gal)
Posted file andreas patch (obsolete) —
I am busy with my day job, probably not time to hack on this.
Flags: needinfo?(gal)
to Vincent Liu since Vincent's working on it
1.3T+ to enable debugging for stability tests
Assignee: nobody → vliu
blocking-b2g: 1.3T? → 1.3T+
We wait for this patch.
Component: General → Gaia::TestAgent
My tarako build seems got corruption and currently try to clone it to look into this issue.
Blocks: 979107
Any progress here? please ask for help if you are stuck.
Blocks: 984095
Blocks: 984101
(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.
Flags: needinfo?(styang) → needinfo?(vliu)
From Keven Kuo, Vincent Liu and Alan are working on it and will finish it by tomorrow.
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.
Flags: needinfo?(vliu)
Component: Gaia::TestAgent → General
Component: General → Breakpad Integration
Product: Firefox OS → Toolkit
Version: unspecified → 28 Branch
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
Posted patch WIP-v1.patch (obsolete) — Splinter Review
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)
Posted patch WIP-v2.patch (obsolete) — Splinter Review
(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.
Attachment #8393338 - Attachment is obsolete: true
Posted patch bug-983022-fix.patch (obsolete) — Splinter Review
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
Attachment #8393414 - Attachment is obsolete: true
Attachment #8394570 - Flags: review?(ehsan)
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.
Attachment #8394570 - Flags: review?(ehsan) → review?(ted)
We need a way to enable/disable this. This shouldn't be enabled on production devices.
Comment on attachment 8394570 [details] [diff] [review]
bug-983022-fix.patch

Please allow this to be enabled/disabled via a prop.
Attachment #8394570 - Flags: review?(ted) → review-
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.
Attachment #8394570 - Flags: review?(ted)
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
Attachment #8394570 - Flags: review?(ted) → review+
How can we do this in the harness?
(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.
(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.
triage: minus, this will not block release. 
this is helpful for debugging stability bugs. please ask for approval to land in 1.3T
blocking-b2g: 1.3T+ → -
Posted patch bug-983022-fix-v2.patch (obsolete) — Splinter Review
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.
Attachment #8394570 - Attachment is obsolete: true
Attachment #8396946 - Flags: review?(ted)
Attachment #8396946 - Flags: review?(gal)
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.
Attachment #8396946 - Flags: review?(gal) → review+
Attachment #8396946 - Flags: review?(ted) → review+
Posted patch bug-983022-v3.patch (obsolete) — Splinter Review
Based on review suggestion on Comment 24, the bug-983022-v3.patch was created.
Attachment #8396946 - Attachment is obsolete: true
Attachment #8399262 - Flags: review?(gal)
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
Attachment #8399262 - Flags: review?(gal) → review+
Posted patch bug-983022-fix-v4.patch (obsolete) — Splinter Review
> 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.
Attachment #8399262 - Attachment is obsolete: true
Attachment #8400501 - Flags: review?(gal)
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 }
Attachment #8400501 - Flags: review?(gal) → review+
(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.
The attached patch is proposed to land.
Attachment #8390304 - Attachment is obsolete: true
Attachment #8390316 - Attachment is obsolete: true
Attachment #8400501 - Attachment is obsolete: true
Keywords: checkin-needed
Hi Joe,

The patch is proposed to land to v1.3T branch. Do we need mark it as 1.3T+ to land it?
blocking-b2g: - → 1.3T?
Flags: needinfo?(jcheng)
triage: before we have the approval 1.3T flag ready, i will 1.3T+ for getting this into tarako. thanks
blocking-b2g: 1.3T? → 1.3T+
Flags: needinfo?(jcheng)
Flags: needinfo?(ttsai)
(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
Flags: needinfo?(ryanvm)
Whiteboard: [tarako_only]
Whiteboard: [tarako_only] → [tarako_only][eta:20140411]
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/eef32863a798
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
Whiteboard: [tarako_only][eta:20140411] → [tarako_only]
You need to log in before you can comment on or make changes to this bug.