672681 - (asyncAddDownload) addDownload should be made asynchronous

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 1

[Marco Bonardo [:mak]]

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

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[:Paolo Amadini]

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?

Comment 4

[:Paolo Amadini]

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.

Comment 5

[:Paolo Amadini]

Attachment: Desktop patch

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.

Comment 6

[Marco Bonardo [:mak]]

(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).

Comment 7

[Marco Bonardo [:mak]]

(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.

Comment 8

[:Paolo Amadini]

(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).

Comment 9

[:Paolo Amadini]

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.

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Marco Bonardo [:mak]]

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 12

[:Paolo Amadini]

Attachment: Updated patch

Comment 13

[Marco Bonardo [:mak]]

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

Comment 14

[:Paolo Amadini]

Attachment: Final patch

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

Comment 15

[:Paolo Amadini]

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.

Comment 16

[Dietrich Ayala (:dietrich)]

What are the next steps here?

Comment 17

[:Paolo Amadini]

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.

Comment 18

[:Paolo Amadini]

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

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/af0a15a36363

Comment 20

[Mark Banner (:standard8)]

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.

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/af0a15a36363

Comment 22

[Marco Bonardo [:mak]]

Thanks Mark

Comment 23

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDownloadManager#addDownload%28%29

And mentioned on Firefox 12 for developers.