nsNavHistory::AddDownload should be made asynchronous. Per the changes in bug 564916, it calls nsNavHistory::AddVisit which is a synchronous call on the main thread.

thanks, actually the issue exists from a long time, not fault of bug 564916.

Paolo, willing to figure out this bug? it would be another rock removed from being able to deprecate all synchronous history. I think it may use the same IHistory methods used by the docshell.

I'm considering to move the implementation of nsIDownloadHistory and its ContractID to History.cpp, directly leveraging the implementation methods there. This component is not available with Android history, but, if I understand correctly, also the Places database isn't. In fact "test_download_history.js" is disabled on Android. So what is the expected behavior for nsIDownloadHistory with Android history? Is the implementation in docshell's nsDownloadHistory.cpp used at all?

It looks like the asynchronous visit API doesn't add the referring URI to the Places database if it doesn't exist already, while the synchronous AddVisit API does. nsIDownloadHistory::AddDownload used to inherit the behavior of the synchronous API, do you think it's fine if now it implements the other behavior instead? I can update "test_download_history.js" to create the referrer visit beforehand and just test aReferringID on the visit.

This has the referrer behavior of the asynchronous API. Note that the annotation APIs are still on the main thread, which is likely to block until the write transaction on the worker thread is released. This happens because the visited notifications are sent before the transaction is committed (see the InsertVisitedURIs::Run implementation). The main thread will not block anymore when the annotation APIs become asynchronous.

(In reply to Paolo Amadini from comment #3) > I'm considering to move the implementation of nsIDownloadHistory and its > ContractID to History.cpp, directly leveraging the implementation methods > there. May be fine. > This component is not available with Android history, but, if I understand > correctly, also the Places database isn't. In fact "test_download_history.js" > is disabled on Android. Native Android doesn't use Places anymore, they have their own implementation of History.cpp implementing IHistory. I don't know what's the plan related to downloads but I suspect it just doesn't add to history. They may want to implement the new method you are going to add to IHistory if interested, in the meanwhile we may add it with NS_ERROR_NOT_IMPLEMENTED or as a no-op. > Is the implementation in docshell's > nsDownloadHistory.cpp > used at all? At first glance I can't find anything using that code, it's fallback code for implementers having their own globalhistory implementation I suppose. I am not aware of any. New implementers should probably implement IHistory and build without places to have history working, so I suspect it's simpler to just remove this code. If you want to keep it in the meanwhile, you may do like the docshell, check if a IHistory implmentation exists, if not fallback to the old global history (nsIDownloadHistory in your case).

(In reply to Paolo Amadini from comment #4) > It looks like the asynchronous visit API doesn't add the referring URI to the > Places database if it doesn't exist already, while the synchronous AddVisit > API > does. Yes, the new API just relies on VisitURI being invoked when needed. no internal magics. > nsIDownloadHistory::AddDownload used to inherit the behavior of the > synchronous > API, do you think it's fine if now it implements the other behavior instead? > I can update "test_download_history.js" to create the referrer visit > beforehand > and just test aReferringID on the visit. I suppose in the usual use-cases the referrer will be already stored by the docshell. It's fine to fix the test afaict, adding the referrer from the addDownload would not be that correct.

(In reply to Marco Bonardo [:mak] from comment #6) > At first glance I can't find anything using that code, it's fallback code > for implementers having their own globalhistory implementation I suppose. I > am not aware of any. New implementers should probably implement IHistory and > build without places to have history working, so I suspect it's simpler to > just remove this code. I think that's easy and we could do that in a separate bug. For the rest, the current patch should work for all the known users of nsIDownloadHistory (callers just do nothing if the nsIDownloadHistory interface isn't implemented).

By the way, tryserver results here: https://tbpl.mozilla.org/?tree=Try&rev=bbe7bffbeec4 Builds are fine, but I don't understand if any tests are run on Android.

Android debug doesn't run tests, you need opt builds for those.

Comment on attachment 586785 [details] [diff] [review] Desktop patch Review of attachment 586785 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good, some comments to fix, mostly I want to take a second look since I did not have enough time to go deeper in checks ::: toolkit/components/places/History.cpp @@ +1272,5 @@ > + SetDownloadAnnotations(nsIURI* aDestination) > + : mDestination(aDestination) > + , mHistory(History::GetService()) > + { > + NS_PRECONDITION(mDestination, "Must pass a non-null destination!"); always use breaking assertions like MOZ_ASSERT in new code, unless you have really good reasons to use weak ones. Check also that this is created on mainthread. @@ +1275,5 @@ > + { > + NS_PRECONDITION(mDestination, "Must pass a non-null destination!"); > + } > + > + NS_IMETHODIMP HandleError(nsresult aResultCode, mozIPlaceInfo *aPlaceInfo) I think for inline definitions we use NS_IMETHOD @@ +1283,5 @@ > + } > + > + NS_IMETHODIMP HandleResult(mozIPlaceInfo *aPlaceInfo) > + { > + nsresult rv; declare at first use @@ +1325,5 @@ > + DESTINATIONFILENAME_ANNO, > + destinationFileName, > + 0, > + nsIAnnotationService::EXPIRE_WITH_HISTORY > + ); don't swallow errors, if you don't want to bail out either assert or warn, though I suppose a failure here may bail out with error? @@ +2015,5 @@ > +NS_IMETHODIMP > +History::AddDownload(nsIURI* aSource, nsIURI* aReferrer, > + PRTime aStartTime, nsIURI* aDestination) > +{ > + NS_ENSURE_ARG(aSource); MOZ_ASSERT(NS_IsMainThread()); please @@ +2049,5 @@ > + > + mozIStorageConnection* dbConn = GetDBConn(); > + NS_ENSURE_STATE(dbConn); > + > + nsRefPtr<mozIVisitInfoCallback> callback = aDestination use nsCOMPtr since this is an interface ::: toolkit/components/places/nsNavHistory.cpp @@ +1538,5 @@ > // Normally docshell sends the link visited observer notification for us (this > // will tell all the documents to update their visited link coloring). > + // However, for redirects this will not happen and we need to send it > + // ourselves. > + if (newItem && aIsRedirect) { I think we already discussed this in the past, since AddVisit is not yet deprecated or removed, we should not change its behavior for now... it will stay as it is till we get rid of it to avoid regressing add-ons. Indeed you may invoke it from outside with aTransitionType == TRANSITION_DOWNLOAD ::: toolkit/components/places/tests/head_common.js @@ +754,5 @@ > + onPageChanged: function() { }, > + onDeleteVisits: function() { }, > + QueryInterface: XPCOMUtils.generateQI([ > + Ci.nsINavHistoryObserver, > + ]), while moving this, please add a whitespace between function and () since these are anonymous functions, and remove the whitespace between the braces, for code consistency. ::: toolkit/components/places/tests/unit/test_download_history.js @@ +23,2 @@ > */ > +function waitForFirstOnVisit(aCallback) { I'd avoid the "First" part of the name, usually all of our waitFor methods just return at the first notification. @@ +110,5 @@ > + }); > + > + PlacesUtils.history.addVisit(REFERRER_URI, Date.now() * 1000, null, > + Ci.nsINavHistoryService.TRANSITION_TYPED, false, > + 0); would be really cool if you may use updatePlaces() API here, one less addvisit point to fix in future... It has a callback too! For future reference, we should try avoiding any call to addVisit or addPageWithDetails or any other API that adds a visit synchronously in new code.

Comment on attachment 587979 [details] [diff] [review] Updated patch Review of attachment 587979 [details] [diff] [review]: ----------------------------------------------------------------- It looks pretty good, please just get a Try server run, and we should be done here, thank you! ::: toolkit/components/places/tests/unit/test_download_history.js @@ +29,5 @@ > +function waitForOnVisit(aCallback) { > + let historyObserver = { > + __proto__: NavHistoryObserver.prototype, > + onVisit: function HO_onVisit(aURI, aVisitID, aTime, aSessionID, > + aReferringID, aTransitionType, aGUID, aAdded) { You may probably avoid labeling each argument since you just use arguments

Tryserver build running here: https://tbpl.mozilla.org/?tree=Try&rev=9d58a64a7b96

Setting dev-doc-needed and addon-compat. Now users of nsIDownloadHistory cannot rely on the history entry to be present after the addDownload method returns. They should use nsINavHistoryObserver or another technique to wait for the target URI to be visited, if needed. The "link-visited" observer topic cannot be used because it is notified before the history entry is saved.

What are the next steps here?

Sorry, this was in the checkin-needed limbo. I still had to manually verify that the optimized Android build I started a while ago passed all tests with this change (<https://tbpl.mozilla.org/?tree=Try&rev=849b8b313c8b>). Just to verify that moving the interface implementation doesn't break mobile builds.

By the latest desktop tryserver run was probably affected by the intermittent orange in bug 706897, recently fixed.

I pushed a unit test only bustage fix for this: https://hg.mozilla.org/mozilla-central/rev/8c664c7ecb79 The test was broken on Thunderbird because we don't have private browsing there and the function "todo" is not available in xpcshell tests. Additionally the test was clearing the preference for turning places on, but Thunderbird has places disabled by default, so I've changed it to set the preference to true, as we do in head_common.js.

Thanks Mark

Documentation updated: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDownloadManager#addDownload%28%29 And mentioned on Firefox 12 for developers.