removeAllInDirectory() layout test helper method can call successCallback twice
Created attachment 87002 [details] Patch
Comment on attachment 87002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87002&action=review > LayoutTests/http/tests/filesystem/resources/fs-test-util.js:34 > } nit: duplicated diff file entry? > LayoutTests/http/tests/filesystem/resources/fs-test-util.js:-37 > - else if (this.entriesCount == 0 && this.successCallback) Handling empty directory case differently is a good idea, but looking at the code again, I'm afraid we have more bugs. Isn't there a chance we could call callbacks in a sequence like: - entriesCallback is called with N entries - entryRemovedCallback is called N times - at the last entry we dispatch successCallback() - entriesCallback is called again with another M entries - entryRemovedCallback is called M times - ... (gush) It is guaranteed that this doesn't happen in the current readDirectory implementation but in theory it looks possible in other embedders. So we need to do something like this? done = false; entryRemovedCallback() { if (--entriesCount == 0 && successCallback && done) { successCallback(); successCallback = null; } } entriesCallback() { ... if (entries.length == 0) { if (entriesCount > 0) done = true; else if (successCallback) successCallback(); } }
Created attachment 87168 [details] Added done bit
Comment on attachment 87002 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87002&action=review >> LayoutTests/http/tests/filesystem/resources/fs-test-util.js:34 >> } > > nit: duplicated diff file entry? Not sure what you mean here; for reasons related to how these tests get run, this code is duplicate in two places now. I can work on fixing that, but I suspect it'll take some changes in the scripts. >> LayoutTests/http/tests/filesystem/resources/fs-test-util.js:-37 >> - else if (this.entriesCount == 0 && this.successCallback) > > Handling empty directory case differently is a good idea, but looking at the code again, I'm afraid we have more bugs. > > Isn't there a chance we could call callbacks in a sequence like: > > - entriesCallback is called with N entries > - entryRemovedCallback is called N times > - at the last entry we dispatch successCallback() > - entriesCallback is called again with another M entries > - entryRemovedCallback is called M times > - ... (gush) > > It is guaranteed that this doesn't happen in the current readDirectory implementation but in theory it looks possible in other embedders. > > So we need to do something like this? > > > done = false; > > entryRemovedCallback() > { > if (--entriesCount == 0 && successCallback && done) { > successCallback(); > successCallback = null; > } > } > > entriesCallback() > { > ... > if (entries.length == 0) { > if (entriesCount > 0) > done = true; > else if (successCallback) > successCallback(); > } > } Did something like this, and got rid of firstEntriesCallback as it's really not needed. See what you think.
Comment on attachment 87168 [details] Added done bit Clearing flags on attachment: 87168 Committed r82204: <http://trac.webkit.org/changeset/82204>
All reviewed patches have been landed. Closing bug.