Looking at the list http://dxr.mozilla.org/mozilla-central/search?q=removePage+ext%3Ajs*&case=true, I see : 1. toolkit/components/places consumers; 2. services/sync consumers; 3. browser/ consumers; 4. addon-sdk/ consumers; 5. tests (bug 1086549).

Comment on attachment 8509419 [details] [diff] [review] 4. browser/components/places Note that I haven't updated PlacesController._removeRange yet.

fwiw I'd suggest to complete bug 1086549 first, so we have better tests coverage

Incidentally, https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=69faad689f06 seems ok.

Comment on attachment 8509416 [details] [diff] [review] 2. services/sync Review of attachment 8509416 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/sync/modules/engines/history.js @@ +224,5 @@ > + break; > + } > + > + blockers.push(new Promise(resolve => { > + let updatePlacesCallback = { Nit: trailing whitespace. (Not your fault!) @@ +233,5 @@ > + handleCompletion: resolve, > + }; > + this._asyncHistory.updatePlaces(records, resolve); > + })); > + } while (true); I've been staring at this for a little while, but I don't see it. Could you explain how this do..while loop terminates? I don't see anything within it that would truncate `records`, which is the only breaking condition. Why do we need the loop at all? @@ +238,5 @@ > + > + // Since `failed` is updated asynchronously and this function is > + // synchronous, we need to spin-wait until we are sure that all > + // updates to `fail` have completed. > + Promise.all(blockers).then(handleAsyncOperationsComplete); This should have a rejection handler so that we don't wait forever!

(In reply to Richard Newman [:rnewman] from comment #9) > @@ +233,5 @@ > > + handleCompletion: resolve, > > + }; > > + this._asyncHistory.updatePlaces(records, resolve); > > + })); > > + } while (true); > > I've been staring at this for a little while, but I don't see it. > > Could you explain how this do..while loop terminates? I don't see anything > within it that would truncate `records`, which is the only breaking > condition. > > Why do we need the loop at all? Oh, good catch - this was meant to be a `while (false)`. I wonder why this passes the tests. Anyway, I'll just remove the `while (false)`. > > @@ +238,5 @@ > > + > > + // Since `failed` is updated asynchronously and this function is > > + // synchronous, we need to spin-wait until we are sure that all > > + // updates to `fail` have completed. > > + Promise.all(blockers).then(handleAsyncOperationsComplete); > > This should have a rejection handler so that we don't wait forever! Good point.

Comment on attachment 8509415 [details] [diff] [review] 1. toolkit/components/places Review of attachment 8509415 [details] [diff] [review]: ----------------------------------------------------------------- Does this pass all the tests? I was expecting us to have a test checking removing an entry from autocomplete really removes it from history... maybe I'm wrong. Please verify on Try. Actually I found test_autocomplete_on_value_removed_479089.js, I'd expect that test to fail and enforce us to wait for a remove notification ::: toolkit/components/places/UnifiedComplete.js @@ +1605,5 @@ > //// nsIAutoCompleteSimpleResultListener > > onValueRemoved: function (result, spec, removeFromDB) { > if (removeFromDB) { > + PlacesUtils.history.remove(spec); .then(null, ex => Cu.reportError); ::: toolkit/components/places/nsPlacesAutoComplete.js @@ +555,5 @@ > > onValueRemoved: function PAC_onValueRemoved(aResult, aURISpec, aRemoveFromDB) > { > if (aRemoveFromDB) { > + PlacesUtils.history.remove(aURISpec); ditto!

ps: even if it doesn't fail, it may be intermittent.

Comment on attachment 8509417 [details] [diff] [review] 3. downloads Review of attachment 8509417 [details] [diff] [review]: ----------------------------------------------------------------- Also here you should handle rejections through reportError... and I don't recall if we have tests for this code path, it's likely we don't.

Comment on attachment 8509419 [details] [diff] [review] 4. browser/components/places Review of attachment 8509419 [details] [diff] [review]: ----------------------------------------------------------------- Looks like you forgot a RemovePage() entry in controller.js? http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/controller.js#896 ::: browser/components/places/content/controller.js @@ +982,5 @@ > this._removeHistoryContainer(node); > } > } > > + PlacesUtils.history.remove([...URIs.values()]); I think .values() is unneeded. It might still be worth to do this in chunks if there's no "breathing" between notifications and until we don't have onDeletePages... I think you send notifications in a loop, and those will block main-thread until done. Just try removing 10 thousands entries, and see. PS: if we remove chunking we should also remove REMOVE_PAGES_CHUNKLEN const.

Comment on attachment 8509417 [details] [diff] [review] 3. downloads Review of attachment 8509417 [details] [diff] [review]: ----------------------------------------------------------------- Need to handle rejections. Also, can you test what happens if the action is triggered two times before the notification of the deletion reaches us? I believe this can realistically only happen programmatically, or if multiple OS click events are somehow queued, so an error message the second time would be fine, as long as we don't have major breakage. ::: browser/components/downloads/content/allDownloadsViewOverlay.js @@ +668,5 @@ > if (this._dataItem) > this._dataItem.remove(); > + if (this._placesNode) { > + // Remove asynchronously from history. > + PlacesUtils.history.remove(this._downloadURIObj); .catch(Cu.reportError) ::: browser/components/downloads/content/downloads.js @@ +1444,5 @@ > cmd_delete: function DVIC_cmd_delete() > { > this.dataItem.remove(); > + // Remove asynchronously from history. > + PlacesUtils.history.remove(this.dataItem.uri); .catch(Cu.reportError)

(In reply to :Paolo Amadini from comment #16) > Comment on attachment 8509417 [details] [diff] [review] > 3. downloads > > Review of attachment 8509417 [details] [diff] [review]: > ----------------------------------------------------------------- > > Need to handle rejections. Also, can you test what happens if the action is > triggered two times before the notification of the deletion reaches us? > > I believe this can realistically only happen programmatically, or if > multiple OS click events are somehow queued, so an error message the second > time would be fine, as long as we don't have major breakage. Attempting to remove a URI that doesn't exist using `PlacesUtils.history.remove` resolves to `false` instead of `true`, and won't notify observers because nothing has been removed, but it will not break. I think that's sufficient, or am I missing something? > > ::: browser/components/downloads/content/allDownloadsViewOverlay.js > @@ +668,5 @@ > > if (this._dataItem) > > this._dataItem.remove(); > > + if (this._placesNode) { > > + // Remove asynchronously from history. > > + PlacesUtils.history.remove(this._downloadURIObj); > > .catch(Cu.reportError) Well, the default error handler is going to provide better error reporting, plus it will cause test failures, so I'd rather keep it.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #17) > Attempting to remove a URI that doesn't exist using > `PlacesUtils.history.remove` resolves to `false` instead of `true`, and > won't notify observers because nothing has been removed, but it will not > break. I think that's sufficient, or am I missing something? It's sufficient. Thanks! > Well, the default error handler is going to provide better error reporting, > plus it will cause test failures, so I'd rather keep it. I think the best practice is still to catch all Promise errors and report them early, instead of waiting for GC. I also think Cu.reportError actually gives better reporting. Also, it makes it clear that you didn't just forget to do something with the return value. The fact is that we should fail tests on newly introduced Cu.reportError calls, wherever they happen. If this isn't possible because of existing failures, didn't we talk about doing that in limited circumstances, for example only when called from "then" callbacks, and then expand that whitelist of circumstances over time? Anyway, probably this is a discussion to have in another bug.

(In reply to :Paolo Amadini from comment #18) > > Well, the default error handler is going to provide better error reporting, > > plus it will cause test failures, so I'd rather keep it. > > I think the best practice is still to catch all Promise errors and report > them early, instead of waiting for GC. I also think Cu.reportError actually > gives better reporting. Also, it makes it clear that you didn't just forget > to do something with the return value. Cu.reportError only keeps the message and loses everything else (we have neither the stack, nor the line number/file name of the error, nor that of the call to Cu.reportError), so using Cu.reportError turns a useful report into non-actionable noise. Also, we cannot use Cu.reportError to fail tests, for the reasons you mention below. By opposition, waiting for GC (or the replacement based on bug 1083361) can keep all this information. Now, I agree that we should do even better. That's bug 1080457, which I hope to land soonish, and which I hope will replace `Cu.reportError` in time, but it hasn't been reviewed yet.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #19) > Cu.reportError only keeps the message and loses everything else (we have > neither the stack, nor the line number/file name of the error, nor that of > the call to Cu.reportError) This is definitely an issue. Hm, I wonder what ".catch(console.error)" would do instead? I still think that explicitly terminating Promise chains is the way to go, not least for making it clear you didn't forget to "return" or "catch", but if neither Cu.reportError nor console.error give us good reporting, then feel free to leave the chain open for lack of a better alternative. > Now, I agree that we should do even better. That's bug 1080457, which I hope > to land soonish, and which I hope will replace `Cu.reportError` in time, but > it hasn't been reviewed yet. I'd then appreciate a comment in the code telling that ".catch(RuntimeAssert.error)" or similar should be added as soon as bug 1080457 lands.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #13) > ps: even if it doesn't fail, it may be intermittent. For some reason, it passed. But yeah, I fixed the tests anyway.

Applied feedback.

Comment on attachment 8512562 [details] [diff] [review] 2. services/sync, v2 Waiting for an updated patch, so clearing flag.

Comment on attachment 8512741 [details] [diff] [review] 5. History.remove now notifies observers by chunks Review of attachment 8512741 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/History.jsm @@ +106,5 @@ > +/** > + * Whenever we update or remove numerous pages, it is preferable > + * to yield time to the main thread every so often to avoid janking. > + * This constant determines the maximal number of notifications we > + * may emit before we should yield. s/should// @@ +455,5 @@ > `); > } > > // 5. Notify observers. > + let chunkSize = 0; s/chunkSize/currentChunkSize/ @@ +469,5 @@ > + if (++chunkSize > NOTIFICATION_CHUNK_SIZE) { > + // Every few notifications, yield time back to the main > + // thread to avoid jank. > + yield Promise.resolve(); > + chunkSize = 0; you could let chunkSize grow indefinitely and use if (++chunkSize % NOTIFICATION_CHUNK_SIZE == 0)

Applied feedback.

The right patch, this time.

Applied feedback. I also introduced Cu.reportError, but hoping that bug 1080457 will let us replace it soon.

Applied feedback, including Cu.reportError, hoping that we can replace it soon.

Comment on attachment 8514927 [details] [diff] [review] 2. services/sync, v3 Review of attachment 8514927 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me!

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c78f4a5b87ef

Hi Yoric, part 3 failed to apply: patching file browser/components/downloads/content/allDownloadsViewOverlay.js Hunk #1 FAILED at 663 1 out of 1 hunks FAILED -- saving rejects to file browser/components/downloads/content/allDownloadsViewOverlay.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh downloads.diff could you take a look? thanks!

Mmmhh... Tests used to pass, but apparently don't pass anymore. I guess I'm conflicting with something. I'll fix this before I hit checkin-needed.

No time to work on this atm. Feel free to steal.

Comment on attachment 8514926 [details] [diff] [review] 5. History.remove now notifies observers by chunks, v2 This part landed via bug 1076775.

Thank you Mark, this greatly helps my todo queue!

MozReview-Commit-ID: CA0g8ccQNB4

MozReview-Commit-ID: KpXAh74W12E

MozReview-Commit-ID: GcVajWOyvkT

Comment on attachment 8847993 [details] [diff] [review] Convert Places JS clients of RemovePage(s) to History.remove; Patch by Yoric, updated by Standard8. r=mak Updated patch, includes test fixes.

Comment on attachment 8847994 [details] [diff] [review] Convert Sync JS clients of RemovePage(s) to History.remove;r=rnewman Simple unbitrotted patch.

Comment on attachment 8847995 [details] [diff] [review] Convert Download JS clients of RemovePage(s) to History.remove;r=paolo Simple unbitrotted update.

Comment on attachment 8847996 [details] [diff] [review] Convert browser/components/places JS clients of RemovePage(s) to History.remove; Patch by Yoric, updated by Standard8. r=mak Unbitrotted with additional test fixes.

I did a try push of the updated patches here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96b61a177a60f9c43212481907009e8cb4ca706e

Comment on attachment 8847993 [details] [diff] [review] Convert Places JS clients of RemovePage(s) to History.remove; Patch by Yoric, updated by Standard8. r=mak Review of attachment 8847993 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/UnifiedComplete.js @@ +2304,5 @@ > // nsIAutoCompleteSimpleResultListener > > onValueRemoved(result, spec, removeFromDB) { > if (removeFromDB) { > + PlacesUtils.history.remove(spec); please add a .catch(Cu.reportError); ::: toolkit/components/places/tests/unifiedcomplete/test_autocomplete_on_value_removed_479089.js @@ +19,5 @@ > + onEndUpdateBatch: function PEX_onEndUpdateBatch() {}, > + onClearHistory() {}, > + onVisit() {}, > + onTitleChanged() {}, > + onDeleteURI(aURI, aGUID, aReason) { shorter form: let obs = new NavHistoryObserver(); obs.onDeleteURI = (uri, guid, reason) => { PlacesUtils.history.removeObserver(obs); Assert.equalq(uri.spec, testURI.spec, "Deleted URI should be the expected one"); }; PlacesUtils.history.addObserver(obs, false);

Comment on attachment 8847996 [details] [diff] [review] Convert browser/components/places JS clients of RemovePage(s) to History.remove; Patch by Yoric, updated by Standard8. r=mak Review of attachment 8847996 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/content/controller.js @@ +887,5 @@ > PlacesUtils.nodeIsQuery(node.parent) && > PlacesUtils.asQuery(node.parent).queryOptions.queryType == > Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY) { > // This is a uri node inside an history query. > + PlacesUtils.history.removePage(node.uri); .catch(Components.utils.reportError) (cannot use Cu here) @@ +958,5 @@ > this._removeHistoryContainer(node); > } > } > > + PlacesUtils.history.remove([...URIs]); Also here, please add a .catch. I think we need to file a follow-up, because remove is using a plain IN clause and it could easily reach the SQL length limit. Especially because it puts urls into the IN clause, and urls can be large. I'm fine with removing the chunking here, though we have to add it directly into the API implementation (even simple chunked calls to the internal remove() would do). Without that, there's concrete risk that people will try to remove thousands entries, and remove will just throw on SQL limit. We could probably increase chunking to 500 or 1k, since now part of the work happens off the main-thread. ::: browser/components/places/tests/chrome/test_bug549192.xul @@ +48,5 @@ > + onEndUpdateBatch: function PEX_onEndUpdateBatch() {}, > + onClearHistory() {}, > + onVisit() {}, > + onTitleChanged() {}, > + onDeleteURI(aURI, aGUID, aReason) { see the other review for a shorter impl.

Actually, SQLITE_MAX_SQL_LENGTH is 1000000. We limit urls to 2k, but some (bookmarks IIRC) could be over that. That basically means we'd fill up the limit even with just 500 urls. That means we can't chunk at 500 as I was hoping. we'll likely have to retain the current 300 limit.

(the problem will exist at least until someone implements bug 483318 that will allow to directly bind an array to IN clauses)

There is also a discrepancy between what sqlite docs state (1M at https://www.sqlite.org/limits.html) an what the code says (1B at # define SQLITE_MAX_SQL_LENGTH 1000000000). I think the docs have not been updated, since Richard said the limit is 1B at https://bugzilla.mozilla.org/show_bug.cgi?id=1309696#c10. In any case, that only lowers the follow-up priority since it's less likely for someone to hit the limit. But it's still something to do since one day we should lower our sql limit to reduce the likely of stack overflows like bug 1309696.

(In reply to Marco Bonardo [::mak] from comment #47) > Comment on attachment 8847996 [details] [diff] [review] > ::: browser/components/places/tests/chrome/test_bug549192.xul > @@ +48,5 @@ > > + onEndUpdateBatch: function PEX_onEndUpdateBatch() {}, > > + onClearHistory() {}, > > + onVisit() {}, > > + onTitleChanged() {}, > > + onDeleteURI(aURI, aGUID, aReason) { > > see the other review for a shorter impl. Unfortunately the browser chrome tests for places don't have head_common.js included. I'm not sure it is worth trying to import just for that structure.

(In reply to Marco Bonardo [::mak] from comment #47) > I think we need to file a follow-up, because remove is using a plain IN > clause and it could easily reach the SQL length limit. Especially because it > puts urls into the IN clause, and urls can be large. I'm fine with removing > the chunking here, though we have to add it directly into the API > implementation (even simple chunked calls to the internal remove() would do). > Without that, there's concrete risk that people will try to remove thousands > entries, and remove will just throw on SQL limit. Bug 1348330 is the follow-up.

(In reply to Mark Banner (:standard8) from comment #51) > Unfortunately the browser chrome tests for places don't have head_common.js > included. I'm not sure it is worth trying to import just for that structure. Ah sorry, you're right, I missed the test type.

https://hg.mozilla.org/integration/mozilla-inbound/rev/27e1f17f5275d7411dffe9488e5c3b7a05bfd263 Bug 1087255 - Convert Places JS clients of RemovePage(s) to History.remove; Patch by Yoric, updated by Standard8. r=mak https://hg.mozilla.org/integration/mozilla-inbound/rev/1a3e39513a18f97f4362d3c4bc5abbe07f16bb87 Bug 1087255 - Convert Sync JS clients of RemovePage(s) to History.remove;r=rnewman https://hg.mozilla.org/integration/mozilla-inbound/rev/4975b3ae236b2f5842d2b24b17e6911f025123c2 Bug 1087255 - Convert Download JS clients of RemovePage(s) to History.remove;r=paolo https://hg.mozilla.org/integration/mozilla-inbound/rev/7ceee02e947617160e58b44122ea016cea95ac0c Bug 1087255 - Convert browser/components/places JS clients of RemovePage(s) to History.remove; Patch by Yoric, updated by Standard8. r=mak

https://hg.mozilla.org/mozilla-central/rev/27e1f17f5275 https://hg.mozilla.org/mozilla-central/rev/1a3e39513a18 https://hg.mozilla.org/mozilla-central/rev/4975b3ae236b https://hg.mozilla.org/mozilla-central/rev/7ceee02e9476