Bug 672681 (asyncAddDownload)

addDownload should be made asynchronous

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jaws, Assigned: Paolo)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete, main-thread-io})

unspecified
mozilla12
x86
Windows 7
addon-compat, dev-doc-complete, main-thread-io
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [places-next-wanted])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Whiteboard: [places-next-wanted]
(Assignee)

Updated

6 years ago
Depends on: 591289

Updated

6 years ago
Blocks: 699820
Alias: asyncAddDownload
Keywords: main-thread-io
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.
(Assignee)

Updated

6 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
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?
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
Created attachment 586785 [details] [diff] [review]
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.
Attachment #586785 - Flags: review?(mak77)
(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.
(Assignee)

Comment 8

5 years ago
(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).
(Assignee)

Comment 9

5 years ago
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.
Attachment #586785 - Flags: review?(mak77) → review-
(Assignee)

Comment 12

5 years ago
Created attachment 587979 [details] [diff] [review]
Updated patch
Attachment #586785 - Attachment is obsolete: true
Attachment #587979 - Flags: review?(mak77)
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
Attachment #587979 - Flags: review?(mak77) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 588335 [details] [diff] [review]
Final patch

Tryserver build running here: https://tbpl.mozilla.org/?tree=Try&rev=9d58a64a7b96
Attachment #587979 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
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.
Keywords: addon-compat, dev-doc-needed
What are the next steps here?
(Assignee)

Comment 17

5 years ago
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.
Keywords: checkin-needed
(Assignee)

Comment 18

5 years ago
By the latest desktop tryserver run was probably affected by the intermittent
orange in bug 706897, recently fixed.
http://hg.mozilla.org/integration/mozilla-inbound/rev/af0a15a36363
No longer depends on: 564916
Keywords: checkin-needed
Target Milestone: --- → mozilla12
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.
https://hg.mozilla.org/mozilla-central/rev/af0a15a36363
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Thanks Mark
Blocks: 741175
Documentation updated:

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

And mentioned on Firefox 12 for developers.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.