Closed Bug 1265420 Opened 4 years ago Closed 4 years ago

SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch

Categories

(Toolkit :: Places, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file)

We'd like to have this API, so a caller can abort an ongoing network fetch.
Priority: -- → P1
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Summary: SetAndFetchFaviconForPage should accept a cancelable object to abort the fetching → SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch
Comment on attachment 8747277 [details]
MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49801/diff/1-2/
Comment on attachment 8747277 [details]
MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49801/diff/2-3/
Comment on attachment 8747277 [details]
MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49801/diff/3-4/
Comment on attachment 8747277 [details]
MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49801/diff/4-5/
Attachment #8747277 - Attachment description: MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r= → MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=gijs
Attachment #8747277 - Flags: review?(gijskruitbosch+bugs)
note that I didn't add a test because the correct functionality of icons is already tested, testing the correct functionality of canceling is complicated by a couple things:
1. need to simulate a slow favicon load from the network, not sure how complex this is with the available utils
2. bug 740457 makes so that we only callback on a successful fetch, that means the test should rely on timeouts, and would be slow.

I'd say at least we'd want bug 740457 to write a meaningful test, so I'd rather leave it for that bug. Otherwise it would be more expensive than the benefit (in the end this is just invoking nsIRequest->Cancel() that is surely already tested).

I also tried to simplify the class hierarchies in AsyncFaviconHelpers, some of the classes were only invoked by another class, so it made sense to merge them and the base class was not really useful anymore. That made most of the destructors pointless and allowed to add some final/explicit annotations. There is more I could do, but I'll leave that for when we start implementing hi-dpi icons support.
(regardless, I had to merge at least the fetching from db/network classes to be able to pass a class handle out for Cancel)
Attachment #8747277 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8747277 [details]
MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=Gijs

https://reviewboard.mozilla.org/r/49801/#review46985

I'm sorry, but I am not comfortable being the sole reviewer of a places C++ patch of this size, unfortunately... there are too many intricate cross-thread-thingies going on for me to be sure of what I'm doing (e.g. no longer creating our own pointer for the callback and the removal of the comment there... I don't understand why that changed? There's a lot of assertions changing as well where I'm not sure if the net effect is the same or not. Also not sure if the added mainthread assertions are always OK in e.g. the docshell caller case.). Perhaps Felipe or someone else can help?

A more general question: was there a particular reason to write our own interface rather than just using nsICancelable ? We could forward the reason passed to the request's Cancel() method. Feels OK to move that responsibility to callers.

::: toolkit/components/places/AsyncFaviconHelpers.cpp:360
(Diff revision 5)
>                                     nsIURI* aPageURI,
>                                     enum AsyncFaviconFetchMode aFetchMode,
> -                                   uint32_t aFaviconLoadType,
> +                                   bool aFaviconLoadPrivate,
>                                     nsIFaviconDataCallback* aCallback,
> -                                   nsIPrincipal* aLoadingPrincipal)
> +                                   nsIPrincipal* aLoadingPrincipal,
> +                                   mozIPlacesPendingOperation** _canceler)

Do we still normally verify that consumers passed a null out param here? Maybe I spend too much time in crufty old C++ code and that's why I'm used to seeing it? If it's not just me being outdated, we probably should add that.

::: toolkit/components/places/AsyncFaviconHelpers.cpp:459
(Diff revision 5)
>    else {
> -    // Fetch the icon from network.  When done this will associate the
> -    // icon to the page and notify.
> -    RefPtr<AsyncFetchAndSetIconFromNetwork> event =
> -      new AsyncFetchAndSetIconFromNetwork(mIcon, mPage, mFaviconLoadPrivate,
> +    // Fetch the icon from the network, the request starts from the main-thread.
> +    // When done this will associate the icon to the page and notify.
> +    nsCOMPtr<nsIRunnable> event =
> +      NS_NewRunnableMethod(this, &AsyncFetchAndSetIconForPage::FetchFromNetwork);
> -                                          mCallback, mLoadingPrincipal);
> -
> -    // Start the work on the main thread.
>      rv = NS_DispatchToMainThread(event);
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  
>    return NS_OK;

Feels like we could take this out of the else (no else after return) and `return NS_DispatchToMainThread(event);`, and omit NS_ENSURE_SUCCESS, to make this slightly simpler.

::: toolkit/components/places/AsyncFaviconHelpers.cpp:519
(Diff revision 5)
> +  if (mCanceled) {
> +    return NS_ERROR_UNEXPECTED;
> +  }

This will throw when called from JS and there's no way to inspect whether or not we've already canceled. Can we just make this a no-op (ie return NS_OK), potentially with NS_WARNING or whatever?
(In reply to :Gijs Kruitbosch from comment #8)
> longer creating our own pointer for the callback and the removal of the
> comment there

It was a "hack" before, it was using .swap to avoid invoking addRef on the wrong thread (the object is not thread safe, should only be addrefed on the main-thread). nsMainThreadPtrHandle solves that problem by wrapping the main-thread oject in a thread-safe smart pointer and then we don't need the workaround.

> There's a lot of
> assertions changing

only changing from non-fatal to fatal.

> Also not sure if the added mainthread assertions are always OK in
> e.g. the docshell caller case.). Perhaps Felipe or someone else can help?

All the Places XPCOM APIs must be invoked on the main-thread. No way out. that's what the fatal assertions are for :)

> A more general question: was there a particular reason to write our own
> interface rather than just using nsICancelable ? We could forward the reason
> passed to the request's Cancel() method. Feels OK to move that
> responsibility to callers.

Not a blocking reason, first I didn't know about nsICancelable. Now that I look at it, we don't need a reason param, and it's not optional in nsICancelable.
I'd like the API to be cleaner for js consumers, so they don't have to pass XPCOM error codes or such.

> ::: toolkit/components/places/AsyncFaviconHelpers.cpp:360
> (Diff revision 5)
> >                                     nsIURI* aPageURI,
> >                                     enum AsyncFaviconFetchMode aFetchMode,
> > -                                   uint32_t aFaviconLoadType,
> > +                                   bool aFaviconLoadPrivate,
> >                                     nsIFaviconDataCallback* aCallback,
> > -                                   nsIPrincipal* aLoadingPrincipal)
> > +                                   nsIPrincipal* aLoadingPrincipal,
> > +                                   mozIPlacesPendingOperation** _canceler)
> 
> Do we still normally verify that consumers passed a null out param here?

yes, I should add an NS_ENSURE_ARG_POINTER there.

> 
> ::: toolkit/components/places/AsyncFaviconHelpers.cpp:519
> (Diff revision 5)
> > +  if (mCanceled) {
> > +    return NS_ERROR_UNEXPECTED;
> > +  }
> 
> This will throw when called from JS and there's no way to inspect whether or
> not we've already canceled. Can we just make this a no-op (ie return NS_OK),
> potentially with NS_WARNING or whatever?

The scope of throwing is to show to the caller that he is doing something wrong (trying to cancel multiple times). This worked pretty well in Storage API so far, I don't think we should make it a no-op cause it would hide bugs on the caller side. We could also expose a canceled attribute in the interface, but I don't know if it's worth it. As I said this kind of API is already in use and never heard of a complain.
I'm moving all the cleanup to bug 1269737.
Comment on attachment 8747277 [details]
MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49801/diff/5-6/
Attachment #8747277 - Attachment description: MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=gijs → MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=Gijs
Attachment #8747277 - Flags: review?(gijskruitbosch+bugs)
Blocks: 1269737
Attachment #8747277 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8747277 [details]
MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=Gijs

https://reviewboard.mozilla.org/r/49801/#review47173

::: toolkit/components/places/AsyncFaviconHelpers.cpp:557
(Diff revision 6)
> +
> +NS_IMETHODIMP
> +AsyncFetchAndSetIconForPage::OnStartRequest(nsIRequest* aRequest,
> -                                                nsISupports* aContext)
> +                                            nsISupports* aContext)
>  {
> +  mRequest = aRequest;

Should we check mCanceled here as well, and call Cancel() on aRequest immediately?
yep, good idea, Cancel could happen in the time between starting the request and receiving onStartRequest, and I should handle that.
Comment on attachment 8747277 [details]
MozReview Request: Bug 1265420 - SetAndFetchFaviconForPage should return a cancelable object to allow aborting the fetch. r=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49801/diff/6-7/
https://hg.mozilla.org/mozilla-central/rev/e8597da96606
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1276636
You need to log in before you can comment on or make changes to this bug.