Open Bug 521264 Opened 15 years ago Updated 10 months ago

don't use file.exists() when not necessary

Categories

(Firefox :: General, defect)

defect

Tracking

()

REOPENED

People

(Reporter: dietrich, Unassigned)

References

(Depends on 2 open bugs, Blocks 1 open bug, )

Details

(Keywords: main-thread-io, Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here] [feature] p=0)

Attachments

(1 file, 12 obsolete files)

3.03 KB, patch
dietrich
: review+
marco
: checkin+
Details | Diff | Splinter Review
if the code is opening a file, instead should just open it and catch the error, instead of the if (exists()) { open() } pattern. exists() results in an extra system call, which can be very expensive on mac and mobile devices.

files to look at:

/uriloader/exthandler/nsHandlerService.js
/toolkit/components/places/src/utils.js
/toolkit/components/url-classifier/content/url-crypto-key-manager.js
/toolkit/components/filepicker/content/filepicker.js
/toolkit/components/passwordmgr/src/storage-mozStorage.js
/toolkit/components/passwordmgr/src/storage-Legacy.js
/toolkit/components/contentprefs/src/nsContentPrefService.js
/toolkit/components/search/nsSearchService.js
/toolkit/mozapps/downloads/src/DownloadLastDir.jsm
/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
/toolkit/mozapps/downloads/content/downloads.js
/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
/toolkit/mozapps/extensions/src/nsBlocklistService.js
/toolkit/mozapps/update/src/nsUpdateService.js.in
/toolkit/mozapps/plugins/content/pluginInstallerService.js
/toolkit/content/contentAreaUtils.js
/toolkit/crashreporter/content/crashes.js
/browser/components/feeds/src/FeedWriter.js
/browser/components/distribution.js
/browser/components/nsBrowserContentHandler.js
/browser/components/sessionstore/src/nsSessionStartup.js
/browser/components/sessionstore/src/nsSessionStore.js
/browser/components/nsBrowserGlue.js
/browser/components/microsummaries/src/nsMicrosummaryService.js
/browser/components/preferences/main.js
/browser/components/preferences/applications.js
/browser/base/content/browser.js
OS: Windows NT → All
Hardware: x86 → All
Whiteboard: [ts][tsnap]
Assignee: nobody → dietrich
Blocks: 447581
I'm taking this. Feel free to reclaim.
Assignee: dietrich → mitchell.field
Status: NEW → ASSIGNED
Assignee: mitchell.field → nobody
Status: ASSIGNED → NEW
Can I do the same to the pattern "if (exists()) { remove() }" ?
Yes, that should be ok, thanks!
Assignee: nobody → mar.castelluccio
Attached patch From browser/base (obsolete) — Splinter Review
I'll create more or less a patch per directory, because there are a lot of files to modify (so there'll be less probability to bitrot something).
Attachment #553534 - Flags: review?(dietrich)
Comment on attachment 553534 [details] [diff] [review]
From browser/base

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

Thanks Marco! However, you're not actually adding any error handling here. Not catching the exceptions thrown from these file operations could cause problems.
Attachment #553534 - Flags: review?(dietrich) → review-
Attached patch From browser/base v2 (obsolete) — Splinter Review
Added error checking.
Attachment #553534 - Attachment is obsolete: true
Attachment #554374 - Flags: review?(dietrich)
Comment on attachment 554374 [details] [diff] [review]
From browser/base v2

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

r=me with the changes below.

::: browser/base/content/safeMode.js
@@ +79,2 @@
>      localstoreFile.remove(false);
> +  } catch(e) {

Instead of just eating this, can you report the error like so:

Components.utils.reportError(e);

::: docshell/test/unit/head_docshell.js
@@ +71,5 @@
>  function cleanup()
>  {
>    // we need to remove the folder that we created for the profile
>    try {
> +    profileDir.remove(true);

should leave this be. we don't care about the unit tests in this case.
Attachment #554374 - Flags: review?(dietrich) → review+
Changes done.
Should I send this to try before the checkin? Or is the patch so simple that it's sure it won't break anything?
Attachment #554374 - Attachment is obsolete: true
Attachment #555888 - Flags: review?(dietrich)
Comment on attachment 555888 [details] [diff] [review]
From browser/base v3

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

r=me. thanks for doing this! looks safe enough to land without try, no behavior change at all really.
Attachment #555888 - Flags: review?(dietrich) → review+
I'll do the other patches in these days. In the meantime could this first patch be checked in?
Keywords: checkin-needed
In my queue :-)
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: [ts][tsnap] → [ts][tsnap] [Don't close yet, more work to do here]
http://hg.mozilla.org/mozilla-central/rev/734bf8fbdb81
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Sorry, there is still work to do here, so reopening.
I'll do the other patches as soon as I can.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Complete patch (WIP) (obsolete) — Splinter Review
This is the try build: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=2fe24aa62e0f
I think only the XPCShellTest could be a problem, but I'm still studying the results (I don't know how to read talos data well).
Attachment #558507 - Flags: review?(dietrich)
The mochitest-other failures are caused by this patch, as is xpcshell. It's likely that the many talos failures are a symptom as well, and I think this is the relevant part:

NOISE: Could not read chrome manifest file '/home/cltbld/talos-slave/talos-data/firefox/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.manifest'.
NOISE: [JavaScript Error: "[Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsILocalFile.remove]"  nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame :: resource:///components/nsUpdateService.js :: aus_gCanApplyUpdates :: line 158"  data: no]" {file: "resource:///components/nsUpdateService.js" line: 160}]
NOISE: [JavaScript Error: "[Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.remove]"  nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)"  location: "JS frame :: resource:///components/nsSessionStore.js :: <TOP_LEVEL> :: line 380"  data: no]" {file: "resource:///components/nsSessionStore.js" line: 382}]
NOISE: [JavaScript Warning: "Use of enablePrivilege is deprecated.  Please use code that runs with the system principal (e.g. an extension) instead." {file: "file:///home/cltbld/talos-slave/talos-data/talos/startup_test/tspaint_test.html?begin=1315313500042" line: 0}]
NOISE: __startTimestamp1315313500807__endTimestamp
NOISE: __startSecondTimestamp1315313501231__endSecondTimestamp
NOISE: __start_report705__end_report
NO
Attached patch Complete patch (obsolete) — Splinter Review
Resolved tests' failures.
Attachment #558507 - Attachment is obsolete: true
Attachment #561170 - Flags: review?(dietrich)
Attachment #558507 - Flags: review?(dietrich)
Comment on attachment 561170 [details] [diff] [review]
Complete patch

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

Hi Marco, sorry for the delay! This patch needs another pass, see comments below. We need to ensure that prior behavior doesn't change.

I know this is very manual work. Feel free to split the patch out into different pieces. Maybe we can find a few different people to make the changes.

::: browser/app/profile/extensions/testpilot@labs.mozilla.com/modules/log4moz.js
@@ +546,5 @@
> +    try {
> +      if(this._file.fileSize < this._maxSize)
> +        return;
> +    }
> +    catch(e) {}

to maintain previous behavior, this should also return if error is that file does not exist. you've also introduced new silent failure for any other error.

the next couple of changes are the same - new silent failure potential, and allowing the code to continue to execute in a failure state.

can you do another pass on the changes to fix these and other places where the changes introduced new silent failures?
Attachment #561170 - Flags: review?(dietrich) → review-
Whiteboard: [ts][tsnap] [Don't close yet, more work to do here] → [snappy]
Whiteboard: [snappy] → [snappy][ts][Don't close yet, more work to do here]
Whiteboard: [snappy][ts][Don't close yet, more work to do here] → [snappy:p4][ts][Don't close yet, more work to do here]
Sorry for the delay, I'll do several patches to ease the review.

> -    if(this._file.exists() &&
> -       this._file.fileSize < this._maxSize)
> -      return;
> +    try {
> +      if(this._file.fileSize < this._maxSize)
> +        return;
> +    }
> +    catch(e if(e.result == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)) {}

Is this ok? This catches the exception only if the file doesn't exist, so it doesn't introduce silent failures.
This looks ok to me(I didn't know we had this exception syntax)
Actually it needs to be:
> +    try {
> +      if(this._file.fileSize < this._maxSize)
> +        return;
> +    }
> +    catch(e if(e.result == NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)) {
> +        return;
> +    }
Otherwise, early return will turn to silent failure.
Sorry, I misread the condition. Please ignore the above comment.
Whiteboard: [snappy:p4][ts][Don't close yet, more work to do here] → [snappy:p3][ts][Don't close yet, more work to do here]
Attached patch From browser/components (obsolete) — Splinter Review
Attachment #561170 - Attachment is obsolete: true
Attachment #579838 - Flags: review?(dietrich)
Attached patch From mobile (obsolete) — Splinter Review
Attachment #579854 - Flags: review?(dietrich)
Attachment #579854 - Flags: review?(dietrich) → review?(mark.finkle)
Comment on attachment 579838 [details] [diff] [review]
From browser/components

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

r=me, thanks! fine to land if try shows tests are passing.

::: browser/components/preferences/applications.js
@@ +1358,4 @@
>  #endif
>  #endif
> +    }
> +    catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) { return false; }

nit: match bracket style with existing code please
Attachment #579838 - Flags: review?(dietrich) → review+
(In reply to Dietrich Ayala (:dietrich) from comment #25)
> r=me, thanks! fine to land if try shows tests are passing.

and the nit fixed too :)
https://tbpl.mozilla.org/?tree=Try&rev=6603836ae6ee (try for the attachment 579838 [details] [diff] [review]).
There are timeout errors on some Jetpack tests, but I think they aren't due to the patch.
https://tbpl.mozilla.org/?tree=Try&rev=30cc03e46fdb (try for the attachment 579854 [details] [diff] [review]).
There are two mochitests failing on Android XUL opt (and not on Android opt). I don't know if they are due to the patch.

Dietrich, could you check the try result for the other patch? There are 8 failing jobs, but I don't know if they are due to the patch or to other problems.
neither of these links work. tbpl can't load the data for some reason.
Comment on attachment 579854 [details] [diff] [review]
From mobile

>diff --git a/mobile/android/components/SessionStore.js b/mobile/android/components/SessionStore.js

>     try {
>-      if (this._sessionFileBackup.exists()) {
>+      try {
>+        this._sessionFileBackup.remove(false);
>         this._shouldRestore = true;
>-        this._sessionFileBackup.remove(false);

This warrants a comment since the order of statements is crucial

  try {
    // Attempt to remove the file first. If it fails, we skip the rest of the block.

>-      if (this._sessionFile.exists()) {
>+      try {
>         // Disable crash recovery if we have exceeded the timeout
>         this._lastSessionTime = this._sessionFile.lastModifiedTime;
>         let delta = Date.now() - this._lastSessionTime;

Needs a comment here too. Move the current comment before the | let delta ... | line and add:

  try {
    // Attempt to access the file first. If it fails, we skip the rest of the block.

These two changes kinda frighten me. Very easy for someone to mess this up afterward. Make sure the comments are clear about the ordering issue.

>   _clearDisk: function ss_clearDisk() {

>+    catch (ex) { dump(ex + '\n'); } // couldn't remove the file - what now?
>+    catch (ex) { dump(ex + '\n'); } // couldn't remove the file - what now?

You can remove theses lines

>diff --git a/mobile/xul/components/AutoCompleteCache.js b/mobile/xul/components/AutoCompleteCache.js

>   init: function init() {

>+    // Load the existing cache or make the empty query cache
>+    if(!this.loadCache())
>       this.fetch(this.query);

if (

>   loadCache: function loadCache() {

>+      return true;
>+    }
>+    catch (e if e.result == Components.results.NS_ERROR_FILE_NOT_FOUND) { return false; }

To match the rest of the style here put the | return false; | on it's own line:

      catch (e if e.result == Components.results.NS_ERROR_FILE_NOT_FOUND) {
        return false;
      } catch (ex) {

>+    catch (ex) {
>       Cu.reportError("AutoCompleteUtils: Could not read from cache file: " + ex);
>     }

I think we want to return false from this branch too

>diff --git a/mobile/xul/components/SessionStore.js b/mobile/xul/components/SessionStore.js

Same as from /mobile/android/components/SessionStore.js

r- so I can see a new patch
Attachment #579854 - Flags: review?(mark.finkle) → review-
I'll do the changes and send the two patches together to try.
Dietrich here is the old try build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mar.castelluccio@studenti.unina.it-6603836ae6ee/
I wonder why tbpl can't load the data.
(In reply to Marco Castelluccio from comment #31)
> I wonder why tbpl can't load the data.

The try repo was reset yesterday, since pushing to try had gotten excruciatingly slow again.
Attached patch From browser/components v2 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=b7e15c0ea0c4

There is only an orange with Android XUL on Mochitest b-c that seems related. Mark, what do you think?

Dietrich, I think we can land the patch for browser/components in the meantime.
Attachment #579838 - Attachment is obsolete: true
Attachment #584330 - Flags: review?(dietrich)
Attached patch From mobile v2 (obsolete) — Splinter Review
Attachment #579854 - Attachment is obsolete: true
Comment on attachment 584330 [details] [diff] [review]
From browser/components v2

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

r=me with these changes, thanks!

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +408,2 @@
>        }
>        catch (ex) { Cu.reportError(ex); } // file was write-locked?

remove the nested try. instead should catch file-not-found and the catchall on the removal. eg:

try { remove ... }
catch{}
catch{}
try { copy... }
catch{}

@@ +3710,2 @@
>      }
> +    catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {}

i think we still want to report any other exception here. can you add the exception reporting back as a second catch, and instead of dump(), use Cu.reportError()? (for this and below)
Attachment #584330 - Flags: review?(dietrich) → review+
Attached patch From browser/components v3 (obsolete) — Splinter Review
Attachment #584330 - Attachment is obsolete: true
Attachment #591251 - Flags: review?(dietrich)
Attachment #555888 - Flags: checkin+
Attached patch From mobile v3 (obsolete) — Splinter Review
Attachment #584331 - Attachment is obsolete: true
Attachment #591254 - Flags: review?(mark.finkle)
Comment on attachment 591251 [details] [diff] [review]
From browser/components v3

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

thanks!
Attachment #591251 - Flags: review?(dietrich) → review+
Attachment #591254 - Flags: review?(mark.finkle) → review+
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here] → [snappy:p3][ts][Don't close yet, more work to do here][autoland]
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here][autoland] → [snappy:p3][ts][Don't close yet, more work to do here][autoland-in-queue]
Autoland Patchset:
	Patches: 555888, 591251, 591254
	Branch: mozilla-central => try
Error applying patch 555888 to mozilla-central.
patching file browser/base/content/browser.js
Hunk #1 FAILED at 2207
Hunk #2 FAILED at 2233
2 out of 2 hunks FAILED -- saving rejects to file browser/base/content/browser.js.rej
patching file browser/base/content/safeMode.js
Hunk #1 FAILED at 69
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/safeMode.js.rej
abort: patch failed to apply

Could not apply and push patchset:
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here][autoland-in-queue] → [snappy:p3][ts][Don't close yet, more work to do here]
Hi Marco, can you un-bitrot this? We can then push again, and finally get this landed :)
(In reply to Dietrich Ayala (:dietrich) from comment #40)
> Hi Marco, can you un-bitrot this? We can then push again, and finally get
> this landed :)

Dietrich, the two patches that need to land aren't bitrotten. The problem is that the releng bot tried to land the first patch, that has already landed before.
Thanks Marco. Autolanding with only the two unlanded patches now.
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here] → [snappy:p3][ts][Don't close yet, more work to do here][autoland:591251,591254]
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here][autoland:591251,591254] → [snappy:p3][ts][Don't close yet, more work to do here][autoland-in-queue]
Autoland Patchset:
	Patches: 591251, 591254
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=b6f85c37d41f
Try run started, revision b6f85c37d41f. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=b6f85c37d41f
Try run for b6f85c37d41f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b6f85c37d41f
Results (out of 211 total builds):
    success: 169
    warnings: 28
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-b6f85c37d41f
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here][autoland-in-queue] → [snappy:p3][ts][Don't close yet, more work to do here]
Test failures, consistent across all platforms:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug580956.js | Undo Close Tab should be enabled.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug609700.js | Exception thrown - [Exception... "'Illegal value' when calling method: [nsISessionStore::duplicateTab]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/browser.js :: duplicateTabIn :: line 12235"  data: no]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug623893.js | an unexpected uncaught JS exception reported through window.onerror - uncaught exception: [Exception... "'Illegal value' when calling method: [nsISessionStore::duplicateTab]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/browser.js :: duplicateTabIn :: line 12235"  data: no] at :0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug623893.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_locationBarCommand.js | Urlbar reverted to original value - Got data:text/plain,3, expected
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_locationBarCommand.js | page proxy state must be invalid for go button to be visible - Got valid, expected invalid
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_locationBarCommand.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_tabMatchesInAwesomebar.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/startup/tests/browser/browser_crash_detection.js | an unexpected uncaught JS exception reported through window.onerror - uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch2.getIntPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://mochitests/content/browser/toolkit/components/startup/tests/browser/browser_crash_detection.js :: checkLastSuccess :: line 7"  data: no] at :0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/startup/tests/browser/browser_crash_detection.js | Test timed out
Attached patch From browser/components v4 (obsolete) — Splinter Review
There was a missing catch!
Attachment #591251 - Attachment is obsolete: true
Attached patch From mobile v4 (obsolete) — Splinter Review
Patch updated.
Attachment #591254 - Attachment is obsolete: true
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here] → [snappy:p3][ts][Don't close yet, more work to do here][autoland:602610,602611]
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here][autoland:602610,602611] → [snappy:p3][ts][Don't close yet, more work to do here][autoland-try:602610,602611]
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here][autoland-try:602610,602611] → [snappy:p3][ts][Don't close yet, more work to do here][autoland-in-queue]
Autoland Patchset:
	Patches: 602610, 602611
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=4a11f8bd1cd3
Try run started, revision 4a11f8bd1cd3. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=4a11f8bd1cd3
I needed also unit tests. How can I ask for them on tbpl?
(In reply to Ryan VanderMeulen from comment #50)
> Use TryChooser syntax. http://trychooser.pub.build.mozilla.org/

Is it possible to ask for unit tests on the running try? The builds are complete, I don't want to waste resources asking for another build.
Unfortunately not, you can only do rebuilds (through the Self-Serve BuildAPI).
You will simply need to push to try again, with appropriate try syntax.
Try run for 4a11f8bd1cd3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4a11f8bd1cd3
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-4a11f8bd1cd3
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here][autoland-in-queue] → [snappy:p3][ts][Don't close yet, more work to do here]
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here] → [snappy:p3][ts][Don't close yet, more work to do here][autoland-try:602610,602611:-b do -p all -u all -t none]
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here][autoland-try:602610,602611:-b do -p all -u all -t none] → [snappy:p3][ts][Don't close yet, more work to do here][autoland-in-queue]
Autoland Patchset:
	Patches: 602610, 602611
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=1928017ad9dd
Try run started, revision 1928017ad9dd. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=1928017ad9dd
Try run for 1928017ad9dd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1928017ad9dd
Results (out of 217 total builds):
    exception: 2
    success: 174
    warnings: 40
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-1928017ad9dd
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here][autoland-in-queue] → [snappy:p3][ts][Don't close yet, more work to do here]
I think the failing tests aren't due to the patches. Dietrich could you check?
Looks good, thanks!
Dietrich, I think the code becomes less clear without using file.exists(). Do you think the speedup is worth the worsening of readability?
If you think so, I can continue with the other patches.
Flags: needinfo?(dietrich)
The readability impact of these particular changes doesn't look significant. It's more important to optimize for less (no) main-thread I/O. That being said, rather than only getting rid of exists() calls, it may be better to focus on porting all of these callers to OS.File (https://developer.mozilla.org/en-US/docs/JavaScript_OS.File).
Depends on: 837989
Depends on: 838210
Depends on: 838821
Attachment #602610 - Attachment is obsolete: true
Attachment #602611 - Attachment is obsolete: true
Flags: needinfo?(dietrich)
Depends on: 838889
Whiteboard: [snappy:p3][ts][Don't close yet, more work to do here] → [snappy:p3][ts][Don't close yet, more work to do here] [feature] p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Target Milestone: Firefox 9 → ---
See Also: → 1436584
See Also: → 1708458
Assignee: mcastelluccio → nobody
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 11 votes.
:mossop, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dtownsend)

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Flags: needinfo?(dtownsend)

Hi Marco,
we come across this bug as part of the Toolkit/Firefox::General and we were wondering if this bug is still relevant and should be moved elsewhere or can be closed.

Would you mind to close it if it is not relevant anymore?

Flags: needinfo?(mcastelluccio)

This bug brings back memories :)

I think it's still relevant, not sure how valuable (it depends on where these exists calls are, if they are on the startup path or part of a UI interaction then they are bad because they are main thread IO).

I can't think of a better component, as these calls are spread everywhere.

Flags: needinfo?(mcastelluccio)
You need to log in before you can comment on or make changes to this bug.