Closed Bug 1188999 Opened 4 years ago Closed 4 years ago

LogShake does not fetch about:memory reports

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
FxOS-S4 (07Aug)
Tracking Status
firefox42 --- fixed

People

(Reporter: gerard-majax, Assigned: hobinjk)

References

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

Attached file logcat during logshake
Issuing VolDown+VolUp keycombo, I have all logs EXCEPT the memory report (which is exactly what I was needing ...).

It might have broken around mid-june.
Attached patch bug1188999.patch (obsolete) — Splinter Review
Attachment #8640716 - Flags: review?(lissyx+mozillians)
I'm a bit iffy on how I make the promises infallible, but that seemed better than writing an entire thing around Promise.forEach to create something like Promise.reallyAll

Try build is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0361e5c0031
Comment on attachment 8640716 [details] [diff] [review]
bug1188999.patch

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

Looks good but I think we can improve readability and factorize a few bits.

::: b2g/components/LogShake.jsm
@@ +316,4 @@
>            }
> +          logArrays[file] = LogParser.prettyPrintArray(logArray);
> +          resolve();
> +        }).catch(function() {

Maybe we should at least dump in the console that this has failed ?

@@ +330,5 @@
> +      let readScreenshotPromise = new Promise(function(resolve) {
> +        LogCapture.getScreenshot().then(screenshot => {
> +          logArrays["screenshot.png"] = screenshot;
> +          resolve();
> +        }).catch(function() {

Idem

::: b2g/components/test/unit/test_logshake_readLog_gonk.js
@@ +18,5 @@
> +const Cc = Components.classes;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "volumeService",

I think we are doing this in several tests now, maybe we shoud factorize and add a support file?

@@ +44,5 @@
> +    run_next_test();
> +  });
> +});
> +
> +add_test(function setup_sdcard() {

Specifically to avoid duplicating this :)

@@ +82,5 @@
> +    // both fail under abnormal circumstances. If any test devices are
> +    // configured to not allow both of these this assumption will need to
> +    // change.
> +    let hasScreenshot = logResults.logFilenames.indexOf("screenshot.png") >= 0;
> +    let hasAboutMemory = logResults.logFilenames.findIndex((filename) => {

No need for "(" and ")" around the variable

@@ +86,5 @@
> +    let hasAboutMemory = logResults.logFilenames.findIndex((filename) => {
> +      // Because the filename involves the PID it can not be easily predicted,
> +      // requiring this approximation
> +      return filename.indexOf("about_memory") >= 0;
> +    }) >= 0;

That is a bit hard to read/follow
Attachment #8640716 - Flags: review?(lissyx+mozillians) → review+
Attached patch bug1188999.patch (obsolete) — Splinter Review
Addressed comments, it's a pretty major series of changes
Attachment #8640716 - Attachment is obsolete: true
Attachment #8640796 - Flags: review?(lissyx+mozillians)
Comment on attachment 8640796 [details] [diff] [review]
bug1188999.patch

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

r+ except the ok( || ), I think we want both, not maybe both.

::: b2g/components/test/unit/test_logshake_readLog_gonk.js
@@ +42,5 @@
> +        return;
> +      }
> +      hasAboutMemory = true;
> +    });
> +    ok(hasScreenshot || hasAboutMemory,

Wait, shouldn't we have *both* ? If we only see one, we will again regress this :(
Attachment #8640796 - Flags: review?(lissyx+mozillians) → review+
Attached patch bug1188999.patch (obsolete) — Splinter Review
Attachment #8640796 - Attachment is obsolete: true
Attached patch bug1188999.patch (obsolete) — Splinter Review
It turns out that the test failures were due to Screenshot.get() being unavailable during XPCShell tests. As a result I made the test use only about:memory, which is still a valid test because it is only making sure that there is not a race condition.

Green results from this are on try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=300dcf484402
Attachment #8641789 - Attachment is obsolete: true
Attachment #8643175 - Flags: review?(lissyx+mozillians)
(In reply to James Hobin [:hobinjk] from comment #8)
> Created attachment 8643175 [details] [diff] [review]
> bug1188999.patch
> 
> It turns out that the test failures were due to Screenshot.get() being
> unavailable during XPCShell tests. As a result I made the test use only
> about:memory, which is still a valid test because it is only making sure
> that there is not a race condition.
> 
> Green results from this are on try at
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=300dcf484402

I'm a bit concerned about your fix: using only "about:memory" will for sure hide a regression. I think I landed the screenshot knowing explicitely it was not working, for making sure we don't regress. That was in bug 1092074.
Flags: needinfo?(hobinjk)
Comment on attachment 8643175 [details] [diff] [review]
bug1188999.patch

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

shipit with the comments fixed :)

::: b2g/components/LogShake.jsm
@@ +276,5 @@
>     * Captures and saves the current device logs, returning a promise that will
>     * resolve to an array of log filenames.
>     */
>    captureLogs: function() {
> +    return this.readLogs().then(function(logArrays) {

logArrays => { ... } ?

@@ +297,5 @@
>        Cu.reportError("Unable to get device properties: " + ex);
>      }
>  
>      // Let Gecko perfom the dump to a file, and just collect it
> +    let readAboutMemoryPromise = new Promise(function(resolve) {

resolve => { ... }

@@ +314,5 @@
>            Cu.reportError("Unable to handle about:memory dump: " + ex);
>          }
>          logArrays[file] = LogParser.prettyPrintArray(logArray);
> +        resolve();
> +      }, function(ex) {

ex => { ... }

@@ +324,3 @@
>  
> +    // Wrap the promise to make it infallible
> +    let readScreenshotPromise = new Promise(function(resolve) {

Idem

@@ +327,4 @@
>        LogCapture.getScreenshot().then(screenshot => {
>          logArrays["screenshot.png"] = screenshot;
> +        resolve();
> +      }, function(ex) {

Idem

::: b2g/components/test/unit/test_logshake_gonk.js
@@ +3,5 @@
>   * for Gonk-specific parts
>   */
>  
> +/* jshint moz: true, esnext: true */
> +/* global Cu, LogCapture, LogShake, ok, add_test, run_next_test, dump, setup_logshake_mocks, OS, sdcard */

Maybe too long line

::: b2g/components/test/unit/test_logshake_gonk_compression.js
@@ +3,5 @@
>   * for Gonk-specific parts
>   */
>  
> +/* jshint moz: true, esnext: true */
> +/* global Cc, Ci, Cu, LogCapture, LogShake, ok, add_test, run_next_test, dump, setup_logshake_mocks, OS, sdcard, FileUtils */

Too long too
Attachment #8643175 - Flags: review?(lissyx+mozillians) → review+
Attached patch bug1188999.patchSplinter Review
Updated patch with nits fixed.
Attachment #8643175 - Attachment is obsolete: true
Flags: needinfo?(hobinjk)
(In reply to James Hobin [:hobinjk] from comment #12)
> Try build here:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf23752ef412

X1 timed out once. Let's retrigger it.
(In reply to Alexandre LISSY :gerard-majax from comment #13)
> (In reply to James Hobin [:hobinjk] from comment #12)
> > Try build here:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf23752ef412
> 
> X1 timed out once. Let's retrigger it.

Of all the retriggers only one failed because of the LogShake intermittent, and nearly all of them were green. Flagging checkin-needed so that this race can finally be behind us.
Keywords: checkin-needed
Assignee: nobody → hobinjk
https://hg.mozilla.org/mozilla-central/rev/216e31f8f1d2
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S4 (07Aug)
You need to log in before you can comment on or make changes to this bug.