don't use file.exists() when not necessary
Categories
(Firefox :: General, defect)
Tracking
()
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
Reporter | ||
Updated•15 years ago
|
Reporter | ||
Updated•15 years ago
|
Comment 1•14 years ago
|
||
I'm taking this. Feel free to reclaim.
Updated•13 years ago
|
Comment 2•13 years ago
|
||
Can I do the same to the pattern "if (exists()) { remove() }" ?
Reporter | ||
Comment 3•13 years ago
|
||
Yes, that should be ok, thanks!
Updated•13 years ago
|
Comment 4•13 years ago
|
||
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).
Reporter | ||
Comment 5•13 years ago
|
||
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.
Comment 6•13 years ago
|
||
Added error checking.
Reporter | ||
Comment 7•13 years ago
|
||
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.
Comment 8•13 years ago
|
||
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?
Reporter | ||
Comment 9•13 years ago
|
||
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.
Comment 10•13 years ago
|
||
I'll do the other patches in these days. In the meantime could this first patch be checked in?
Comment 11•13 years ago
|
||
In my queue :-)
Comment 12•13 years ago
|
||
browser/base/ part: http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=e9351f9ab296 http://hg.mozilla.org/integration/mozilla-inbound/rev/734bf8fbdb81
Comment 13•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/734bf8fbdb81
Comment 14•13 years ago
|
||
Sorry, there is still work to do here, so reopening. I'll do the other patches as soon as I can.
Comment 15•13 years ago
|
||
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).
Comment 16•13 years ago
|
||
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
Comment 17•13 years ago
|
||
Resolved tests' failures.
Reporter | ||
Comment 18•13 years ago
|
||
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?
Reporter | ||
Updated•13 years ago
|
Reporter | ||
Updated•13 years ago
|
Updated•13 years ago
|
Comment 19•13 years ago
|
||
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.
Comment 20•13 years ago
|
||
This looks ok to me(I didn't know we had this exception syntax)
Comment 21•13 years ago
|
||
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.
Comment 22•13 years ago
|
||
Sorry, I misread the condition. Please ignore the above comment.
Updated•13 years ago
|
Comment 23•13 years ago
|
||
Comment 24•13 years ago
|
||
Reporter | ||
Updated•13 years ago
|
Reporter | ||
Comment 25•13 years ago
|
||
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
Reporter | ||
Comment 26•13 years ago
|
||
(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 :)
Comment 27•12 years ago
|
||
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.
Comment 28•12 years ago
|
||
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.
Reporter | ||
Comment 29•12 years ago
|
||
neither of these links work. tbpl can't load the data for some reason.
Comment 30•12 years ago
|
||
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
Comment 31•12 years ago
|
||
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.
Comment 32•12 years ago
|
||
(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.
Comment 33•12 years ago
|
||
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.
Comment 34•12 years ago
|
||
Reporter | ||
Comment 35•12 years ago
|
||
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)
Comment 36•12 years ago
|
||
Updated•12 years ago
|
Comment 37•12 years ago
|
||
Reporter | ||
Comment 38•12 years ago
|
||
Comment on attachment 591251 [details] [diff] [review] From browser/components v3 Review of attachment 591251 [details] [diff] [review]: ----------------------------------------------------------------- thanks!
Updated•12 years ago
|
Reporter | ||
Updated•12 years ago
|
Updated•12 years ago
|
Comment 39•12 years ago
|
||
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:
Updated•12 years ago
|
Reporter | ||
Comment 40•12 years ago
|
||
Hi Marco, can you un-bitrot this? We can then push again, and finally get this landed :)
Comment 41•12 years ago
|
||
(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.
Reporter | ||
Comment 42•12 years ago
|
||
Thanks Marco. Autolanding with only the two unlanded patches now.
Updated•12 years ago
|
Comment 43•12 years ago
|
||
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
Comment 44•12 years ago
|
||
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
Updated•12 years ago
|
Reporter | ||
Comment 45•12 years ago
|
||
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
Comment 46•12 years ago
|
||
There was a missing catch!
Updated•12 years ago
|
Updated•12 years ago
|
Updated•12 years ago
|
Comment 48•12 years ago
|
||
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
Comment 49•12 years ago
|
||
I needed also unit tests. How can I ask for them on tbpl?
Comment 50•12 years ago
|
||
Use TryChooser syntax. http://trychooser.pub.build.mozilla.org/
Comment 51•12 years ago
|
||
(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.
Comment 52•12 years ago
|
||
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.
Comment 53•12 years ago
|
||
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
Updated•12 years ago
|
Updated•12 years ago
|
Updated•12 years ago
|
Comment 54•12 years ago
|
||
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
Comment 55•12 years ago
|
||
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
Updated•12 years ago
|
Comment 56•12 years ago
|
||
I think the failing tests aren't due to the patches. Dietrich could you check?
Reporter | ||
Comment 57•12 years ago
|
||
Looks good, thanks!
Comment 58•11 years ago
|
||
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.
Comment 59•11 years ago
|
||
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).
Updated•11 years ago
|
Updated•11 years ago
|
Updated•11 years ago
|
Updated•11 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 60•1 year ago
|
||
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.
Comment 61•1 year ago
|
||
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.
Comment 62•10 months ago
|
||
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?
Comment 63•10 months ago
|
||
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.
Description
•