Use OS.File in about:crashes

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: marco, Assigned: marco)

Tracking

({main-thread-io})

Trunk
mozilla31
main-thread-io
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async:ready][mentor=Yoric][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 712131 [details] [diff] [review]
Patch
Attachment #712131 - Flags: feedback?(adw)

Comment 1

6 years ago
Comment on attachment 712131 [details] [diff] [review]
Patch

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

I think this is OK.

::: toolkit/crashreporter/content/crashes.js
@@ +117,5 @@
> +        return result;
> +      }, function onError() {});
> +    } catch (e) {
> +      if (!(e instanceof OS.File.Error && e.becauseNoSuchFile)) {
> +        throw e;

This pattern of calling DirectoryIterator in a try-catch to get a promise is repeated five times in the patch.  Couldn't you factor it out into a function that returns the promise?

@@ +136,3 @@
>  
> +    if (submittedPromise) {
> +      let submittedEntries = yield submittedPromise;

I'd prefer to keep the bodies of Task functions as small as possible.  The first yield is here, so couldn't the Task be spawned here, at the conditional?  Same question for the other Task in the patch.
Attachment #712131 - Flags: feedback?(adw) → feedback+
Keywords: main-thread-io
What's the status of this bug?
Flags: needinfo?(mar.castelluccio)
De-assigning
Assignee: mar.castelluccio → nobody
Flags: needinfo?(mar.castelluccio)
Whiteboard: [Async][mentor=Yoric][lang=js]

Updated

5 years ago
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async:ready][mentor=Yoric][lang=js]
Marco, do you want to pick up this bug again?
Flags: needinfo?(mar.castelluccio)
(Assignee)

Comment 5

5 years ago
Ok, I'll rewrite the patch.
Assignee: nobody → mar.castelluccio
Flags: needinfo?(mar.castelluccio)
(Assignee)

Comment 6

5 years ago
Created attachment 8402977 [details] [diff] [review]
Patch v2

Rewritten it from scratch, I've avoided the three promises and just do things sequentially.
Attachment #712131 - Attachment is obsolete: true
Attachment #8402977 - Flags: review?(adw)
Comment on attachment 8402977 [details] [diff] [review]
Patch v2

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

just some drive-by comments (sorry I was curious about the patch)

::: toolkit/crashreporter/content/crashes.js
@@ +6,5 @@
>  
>  var reportURL;
>  
> +Cu.import("resource://gre/modules/CrashReports.jsm");
> +Cu.import("resource://gre/modules/CrashSubmit.jsm");

why isn't CrashSubmit.jsm lazyModuleGetter-ed? it has only one use and doesn't look like it's always invoked.

@@ +127,5 @@
>      body.appendChild(row);
>    }
>  }
>  
> +clearReports = function() {

missing a let (though what's the point of not doing function name() here, doesn't look like it's breaking strict)

fwiw you may do:
let clearReports = Task.async(function* () {
  ...
});

though then the caller should handle the resolution, since you can't put a .then here. But that may make a cleaner API

@@ +130,5 @@
>  
> +clearReports = function() {
> +  Task.spawn(function*() {
> +    var bundles = Cc["@mozilla.org/intl/stringbundle;1"].
> +                  getService(Ci.nsIStringBundleService);

Services.strings

@@ +133,5 @@
> +    var bundles = Cc["@mozilla.org/intl/stringbundle;1"].
> +                  getService(Ci.nsIStringBundleService);
> +    var bundle = bundles.createBundle("chrome://global/locale/crashes.properties");
> +    var prompts = Cc["@mozilla.org/embedcomp/prompt-service;1"].
> +                  getService(Ci.nsIPromptService);

Services.prompt

@@ +142,2 @@
>  
> +    let iterator = new OS.File.DirectoryIterator(CrashReports.submittedDir.path);

mixing let and var without a reason, just change other vars inside this function to let?

@@ +144,5 @@
> +    try {
> +      for (let promiseEntry in iterator) {
> +        let entry = yield promiseEntry;
> +
> +        let leaf = entry.name;

nit: filename? :)

@@ +145,5 @@
> +      for (let promiseEntry in iterator) {
> +        let entry = yield promiseEntry;
> +
> +        let leaf = entry.name;
> +        if (leaf.substr(0, 3) == "bp-" &&

leaf.startsWith("bp-")

@@ +146,5 @@
> +        let entry = yield promiseEntry;
> +
> +        let leaf = entry.name;
> +        if (leaf.substr(0, 3) == "bp-" &&
> +            leaf.substr(-4) == ".txt") {

leaf.endsWith(".txt")

@@ +168,5 @@
> +          date = stat.lastModificationDate;
> +        }
> +
> +        let leaf = entry.name;
> +        if (leaf.substr(0, 11) == "InstallTime" &&

startsWith

@@ +181,3 @@
>      }
> +
> +    iterator = new OS.File.DirectoryIterator(CrashReports.pendingDir.path);

nit: may create a cleanupFolder(path, filter) helper and reuse it these 3 times. would save some boilerplate
(Assignee)

Comment 8

5 years ago
(In reply to Marco Bonardo [:mak] from comment #7)
> ::: toolkit/crashreporter/content/crashes.js
> @@ +6,5 @@
> >  
> >  var reportURL;
> >  
> > +Cu.import("resource://gre/modules/CrashReports.jsm");
> > +Cu.import("resource://gre/modules/CrashSubmit.jsm");
> 
> why isn't CrashSubmit.jsm lazyModuleGetter-ed? it has only one use and
> doesn't look like it's always invoked.

I wanted to build a self-contained patch, so I've just "translated" the nsIFile parts to OS.File (this applies to the other comments too).
I can refactor a bit the code if you want.

> 
> @@ +127,5 @@
> >      body.appendChild(row);
> >    }
> >  }
> >  
> > +clearReports = function() {
> 
> missing a let (though what's the point of not doing function name() here,
> doesn't look like it's breaking strict)

At the beginning I was using Task.async, I went back to Task.spawn because of the |then|, but eventually I forgot to modify the line back to "function clearReports()" :D

> fwiw you may do:
> let clearReports = Task.async(function* () {
>   ...
> });
> 
> though then the caller should handle the resolution, since you can't put a
> .then here. But that may make a cleaner API

Good idea.

Comment 9

5 years ago
Comment on attachment 8402977 [details] [diff] [review]
Patch v2

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

Rubber-stamping Marco's review. :-)  r+ with comments addressed, nits at your discretion.

::: toolkit/crashreporter/content/crashes.js
@@ +6,5 @@
>  
>  var reportURL;
>  
> +Cu.import("resource://gre/modules/CrashReports.jsm");
> +Cu.import("resource://gre/modules/CrashSubmit.jsm");

I agree with Marco about lazifying CrashSubmit.  It's only used when you click a submit link.  Task and osfile should be lazified, too, since they're only used in clearReports, which is only called when you click the clear button.

@@ +170,5 @@
> +
> +        let leaf = entry.name;
> +        if (leaf.substr(0, 11) == "InstallTime" &&
> +            date < oneYearAgo &&
> +            leaf != "InstallTime" + buildID) {

Nit: Some refactoring, but it looks like you could avoid getting the date altogether if `leaf != "InstallTime" + buildID` is false.
Attachment #8402977 - Flags: review?(adw) → review+

Comment 10

5 years ago
Of course if you go with Task.async() in the global scope, no need to lazify Task.

Thanks for picking this back up.
(Assignee)

Comment 11

5 years ago
(In reply to Drew Willcoxon :adw from comment #9)
> Comment on attachment 8402977 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8402977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Rubber-stamping Marco's review. :-)  r+ with comments addressed, nits at
> your discretion.
> 
> ::: toolkit/crashreporter/content/crashes.js
> @@ +6,5 @@
> >  
> >  var reportURL;
> >  
> > +Cu.import("resource://gre/modules/CrashReports.jsm");
> > +Cu.import("resource://gre/modules/CrashSubmit.jsm");
> 
> I agree with Marco about lazifying CrashSubmit.  It's only used when you
> click a submit link.  Task and osfile should be lazified, too, since they're
> only used in clearReports, which is only called when you click the clear
> button.

I think we usually avoid to lazy load Task.jsm and osfile.jsm because they're assumed to be loaded right at startup.
(Assignee)

Comment 12

5 years ago
Created attachment 8404113 [details] [diff] [review]
Patch v3

I did a small refactoring and added a test to ensure no regressions happen.
Attachment #8402977 - Attachment is obsolete: true
Attachment #8404113 - Flags: review?(adw)

Comment 13

5 years ago
Comment on attachment 8404113 [details] [diff] [review]
Patch v3

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

I learned some things from this patch.  Thanks also for the test.

::: toolkit/crashreporter/content/crashes.js
@@ +157,5 @@
> +
> +  yield cleanupFolder(CrashReports.submittedDir.path, function*(aEntry) {
> +    if (aEntry.name.startsWith("bp-") &&
> +        aEntry.name.endsWith(".txt")) {
> +      return true;

return aEntry.name.startsWith("bp-") &&
       aEntry.name.endsWith(".txt");

::: toolkit/crashreporter/test/browser/browser_clearReports.js
@@ +1,1 @@
> +function clickClearReports(tab, cb) {

This file needs a license header.

@@ +17,5 @@
> +    for (let mutation of mutations) {
> +      if (mutation.type == "attributes" &&
> +          mutation.attributeName == "style") {
> +        is(style.display, "none", "Clear reports button hidden");
> +        cb();

observer.disconnect();

@@ +34,5 @@
> +
> +var promptShown = false;
> +
> +let oldPrompt = Services.prompt;
> +Services.prompt = {

Add a registerCleanupFunction right here for restoring oldPrompt instead of waiting until the end of test().

@@ +85,5 @@
> +  report4.lastModifiedTime = Date.now() - 63172000000;
> +
> +  let tab = gBrowser.selectedTab = gBrowser.addTab("about:blank");
> +  let browser = gBrowser.getBrowserForTab(tab);
> +  let onLoad = function() {

function onLoad() {
Attachment #8404113 - Flags: review?(adw) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 8405265 [details] [diff] [review]
Patch v3

Carrying r+.
Attachment #8404113 - Attachment is obsolete: true
Attachment #8405265 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/597f5b9dba63
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [Async:ready][mentor=Yoric][lang=js] → [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/597f5b9dba63
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Async:ready][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async:ready][mentor=Yoric][lang=js]
Target Milestone: --- → mozilla31
A follow-up to this bug is bug 946645. We already established a JSM for interacting with crash files. Ideally about:crashes would be rewritten to use that API.
You need to log in before you can comment on or make changes to this bug.