At the moment, History.cpp requires user to hold strong references to mozIVisitInfoCallback, which is awkward and error-prone. We should rather use nsMainThreadPtrHandle. http://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/History.cpp?from=History.cpp#780-785

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #0) > At the moment, History.cpp requires user to hold strong references to > mozIVisitInfoCallback, which is awkward and error-prone. We should rather > use nsMainThreadPtrHandle. > > http://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/ > History.cpp?from=History.cpp#780-785 I'd expect mozIVisitInfoCallback to be usually a js consumer, could you make a simple example where it's complicated or error-prone to keep a strong reference?

My main point is that using nsMainThreadPtrHandle<> - improves the documentation; - decreases the surprise factor when looking at this code, hence makes it easier to modify; - simplifies the code; - removes the need for a few Big Scary Warnings. Right now, even if we assume we have a JS consumer (and I'm not sure we can), the code paths are sophisticated enough that I'm not entirely confident running the GC at an awkward time on an edge case won't cause a very hard to pinpoint segfault, so if we can tweak the code to make it easier for me to trust it, I believe that we should go for it.

my point is that basically all of the code using callbacks acts the same as this one, so it's a well tested coding style. But I'm open to improvements... What you suggest sounds mostly like a way to keep the ownership on main-thread without having to pass ownership around (taking care to not addref/release off the main thread) and then ensure to release on the main-thread thread. The problem is that this involves having to keep around some object that will own the nsMainThreadPtrHandle until it can be released, that could still require added complication. Currently we just let the previous "task" pass ownership and die. Here you should instead have an array of nsMainThreadPtrHandle, that would have an impact on memory usage, even if probably unimportant. you should still pass over a reference to your callback to be able to retrieve it in the array, and you should still release it from the array at the end to free up memory... So I don't see a very interesting win cause the code is very similar but less memory efficient. The current style works fine mostly because we get a callback on main-thread (API) and in the end we will have to call it on main-thread, so those locations look like the simplest points to addref and release. If we wouldn't have an exit point on main-thread then it would be more complicated and maybe not worth it.

Here is a prototype. I haven't finished running the tests, but it looks good so far, and I believe that it simplifies the code nicely. I am probably missing something from your comment 3, but I didn't see the need to manually add strong references, since the nsMainThreadPtrHandle already contains one.

Comment on attachment 8486357 [details] [diff] [review] Prototype Review of attachment 8486357 [details] [diff] [review]: ----------------------------------------------------------------- I forgot some of the underlying issues here, but off-hand I'd say it looks good and I don't see anything blocking us from doing this. ::: toolkit/components/places/History.cpp @@ +684,5 @@ > */ > class NotifyPlaceInfoCallback : public nsRunnable > { > public: > + NotifyPlaceInfoCallback(nsMainThreadPtrHandle<mozIVisitInfoCallback> aCallback, I wonder if it wouldn't be better to pass references around like const nsMainThreadPtrHandle<mozIVisitInfoCallback>& aCallback I'm not sure what's the suggested way to pass it around, looks like the codebase does both. maybe ask developers or ask.mozilla? @@ +805,5 @@ > VisitData place(aURI); > place.guid = aGUID; > + > + nsMainThreadPtrHandle<mozIVisitInfoCallback> callbackHandle; > + callbackHandle = new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback); please add a MOZ_ASSERT(NS_IsMainThread()); on top of CanAddURI, so we are sure this won't try to create the holder on the other thread. any reason to not directly assign on declare? I'd also just call this callback, no complicate naming needed. @@ +846,5 @@ > return NS_ERROR_FAILURE; > } > > + nsMainThreadPtrHandle<mozIVisitInfoCallback> callbackHandle; > + callbackHandle = new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback); ditto @@ +947,5 @@ > NS_ASSERTION(CanAddURI(uri), > "Passed a VisitData with a URI we cannot add to history!"); > #endif > } > } Should probably make InsertVisitedURIs MOZ_FINAL @@ +1265,5 @@ > mozIVisitInfoCallback* aCallback) { > MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread"); > > + nsMainThreadPtrHandle<mozIVisitInfoCallback> callbackHandle; > + callbackHandle = new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback); ditto @@ +1305,5 @@ > , mCallback(aCallback) > , mHistory(History::GetService()) > { > MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread"); > } Should probably make GetPlaceInfo MOZ_FINAL @@ +1890,5 @@ > navHistory->registerEmbedVisit(uri, aPlace.visitTime); > > if (aCallback) { > + nsMainThreadPtrHandle<mozIVisitInfoCallback> callbackHandle; > + callbackHandle = new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback); ditto @@ +2795,5 @@ > // main thread from there to make sure that all database notifications > // and all embed or canAddURI notifications have finished. > if (aCallback) { > + nsMainThreadPtrHandle<mozIVisitInfoCallback> callbackHandle; > + callbackHandle = new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback); ditto and please add the NS_IsMainThread assert to GetPlacesInfo @@ +2929,5 @@ > // main thread from there to make sure that all database notifications > // and all embed or canAddURI notifications have finished. > if (aCallback) { > + nsMainThreadPtrHandle<mozIVisitInfoCallback> callbackHandle; > + callbackHandle = new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback); ditto @@ +2934,4 @@ > > nsCOMPtr<nsIEventTarget> backgroundThread = do_GetInterface(dbConn); > NS_ENSURE_TRUE(backgroundThread, NS_ERROR_UNEXPECTED); > + nsCOMPtr<nsIRunnable> event = new NotifyCompletion(callbackHandle); off-hand I don't remember why NotifyCompletion was not addrefing by itself... just in case please assert NotifyCompletion is always created on mainthread

(In reply to Marco Bonardo [:mak] (needinfo? me) from comment #5) > I'm not sure what's the suggested way to pass it around, looks like the > codebase does both. maybe ask developers or ask.mozilla? Will do. > any reason to not directly assign on declare? Yes, it won't compile :/ Now that I have your feedback on this, I will try harder to find out why. > I'd also just call this callback, no complicate naming needed. Ok. I was following the conventions in the doc of nsMainThreadPtrHandle. > > nsCOMPtr<nsIEventTarget> backgroundThread = do_GetInterface(dbConn); > > NS_ENSURE_TRUE(backgroundThread, NS_ERROR_UNEXPECTED); > > + nsCOMPtr<nsIRunnable> event = new NotifyCompletion(callbackHandle); > > off-hand I don't remember why NotifyCompletion was not addrefing by itself... > > just in case please assert NotifyCompletion is always created on mainthread Ok (and for the rest, too).

Applied feedback, plus asked about nsMainThreadPtrHandle<>& on ask.mo and got an answer.

Comment on attachment 8487858 [details] [diff] [review] Porting History.cpp to nsMainThreadPtr<> Review of attachment 8487858 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/History.cpp @@ +454,5 @@ > return NS_OK; > } > > + nsMainThreadPtrHandle<mozIVisitedStatusCallback> > + callback(new nsMainThreadPtrHolder<mozIVisitedStatusCallback>(aCallback)); so, the previous lines have a wrong indentation, as well as this one... could you please fix them? @@ +476,1 @@ > NS_ENSURE_TRUE(callback, NS_ERROR_OUT_OF_MEMORY); this should be cb @@ +926,5 @@ > > return NS_OK; > } > private: > InsertVisitedURIs(mozIStorageConnection* aConnection, shouldn't the class be MOZ_FINAL? @@ +1742,5 @@ > + * Find the list of entries that may be removed from `moz_places`. > + * > + * Calling this method makes sense only if we are not clearing the entire history. > + * > + * @param aIDsToRemove An array to fill with the list of `place_id` ids to remove. there is no such array here, maybe this is a leftover from another patch

Applied feedback. Try: https://tbpl.mozilla.org/?tree=Try&rev=0f62f7ef9ef1

Wrong bug# in the commit message.

Your Try run is busted?

Sorry, wrong Try: https://tbpl.mozilla.org/?tree=Try&rev=ba5fca615df3