Closed Bug 1356211 Opened 7 years ago Closed 7 years ago

Supports custom callback thread for nsIURILCassifier.asyncClassifyLocalWithTables

Categories

(Toolkit :: Safe Browsing, enhancement, P2)

enhancement

Tracking

()

RESOLVED WONTFIX
Performance Impact high

People

(Reporter: hchang, Assigned: hchang)

References

Details

Attachments

(3 files)

Because of Bug 1345058, we need this API to support custom callback thread.

The callback will be on the main thread now and Bug 1345058 requires this
API can callback on non-main-thread for synchronization purpose.
Assignee: nobody → hchang
Blocks: 1345058
Whiteboard: [qf:p1]
Attachment #8857961 - Flags: review?(amarchesini)
Hi :baku

I just flagged you for reviewing this patch in preparation of Bug 1345058
(as you are the reviewer of Bug 1343425, which is pretty related to this one)

In Bug 1343425, we support nsIURIClassifier.asyncClassifyLocalWithTables in
content process. The parent protocol of that API is PContent so that 
the IPC thread is the main thread.

When I started trying to solve Bug 1345058 (the consumer of Bug 1343425),
I found the current callback mechanism is not sufficient. The reason is,
chances are we might be need to be waiting on the main thread for the
callback result. If the callback can only occur on main thread, 
"waiting on main thread for callback" is impossible. As a result, I decided
to support the custom callback thread for nsIURIClassifier.asyncClassifyLocalWithTables.

In addition to supporting custom callback thread, I also change the parent
protocol to PBackground so that we can support custom callback thread
in the content process. (If we use PContent as the parent protocol, the IPC 
thread will be main thread so we cannot receive IPC message when we are waiting
on the main thread.)

I am not sure if you are available at this point, ni Anthony as well in case
you are too busy to review this and he can coordinate the reviewing things.

Thank you!
Flags: needinfo?(ajones)
Status: NEW → ASSIGNED
Priority: -- → P2
Comment on attachment 8857961 [details]
Bug 1356211 - Part 1: Move PURLClassifierLocal from PContent to PBackground. (interface changes)

https://reviewboard.mozilla.org/r/130004/#review136322

::: commit-message-60fe2:6
(Diff revision 2)
> +Bug 1356211 - Move PURLClassifierLocal from PContent to PBackground and support custom callback thread.
> +
> +This change is for Bug 1345058, where we need the callback to occur off the main thread
> +so that we can be waiting on main thread for the callback. In order to support custom
> +callback thread, the IPC thread cannot be the main thread so the parent protocol of
> +PURLClassifierLocal is changed to PBackground.

I need more information in order to understand this code. URLClassifier is main-thread only, if we want to run the callback on a separate thread, why we cannot just use I/O thread for them?

::: dom/ipc/URLClassifierChild.h:13
(Diff revision 2)
>  #define mozilla_dom_URLClassifierChild_h
>  
>  #include "mozilla/dom/PURLClassifierChild.h"
>  #include "mozilla/dom/PURLClassifierLocalChild.h"
>  #include "nsIURIClassifier.h"
> +#include "mozilla/SyncRunnable.h"

alphabetic order

::: dom/ipc/URLClassifierChild.h:28
(Diff revision 2)
>  
>    void SetCallback(nsIURIClassifierCallback* aCallback)
>    {
>      mCallback = aCallback;
>    }
> +  void SetCallbackThread(nsIThread* aCallbackThread)

Don't use nsIThread but nsIEventTarget

::: dom/ipc/URLClassifierChild.h:52
(Diff revision 2)
> +        // For PURLClassifierChild.
> +        doCallback();
> +      } else {
> +        // Callback on the callback thread.
> +        nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(doCallback);
> +        SyncRunnable::DispatchToThread(mCallbackThread, r);

mCallbackEventTarget->Displach(r.forget(), ...)

::: dom/ipc/URLClassifierParent.h:13
(Diff revision 2)
>  #define mozilla_dom_URLClassifierParent_h
>  
>  #include "mozilla/dom/PURLClassifierParent.h"
>  #include "mozilla/dom/PURLClassifierLocalParent.h"
>  #include "nsIURIClassifier.h"
> +#include "mozilla/SyncRunnable.h"

alphabetic order

::: dom/ipc/URLClassifierParent.h:23
(Diff revision 2)
>  template<typename BaseProtocol>
>  class URLClassifierParentBase : public nsIURIClassifierCallback,
>                                  public BaseProtocol
>  {
>  public:
> +  URLClassifierParentBase()

: mIPCThread(NS_GetCurrentThread())
{
  MOZ_ASSERT(mIPCThread);
}

::: ipc/glue/BackgroundParentImpl.cpp:71
(Diff revision 2)
>  using mozilla::dom::FileSystemRequestParent;
>  using mozilla::dom::MessagePortParent;
>  using mozilla::dom::PMessagePortParent;
>  using mozilla::dom::UDPSocketParent;
> +using mozilla::dom::URLClassifierLocalParent;
> +using mozilla::dom::PURLClassifierLocalParent;

alphabetic order

::: ipc/glue/BackgroundParentImpl.cpp:881
(Diff revision 2)
> +//////////////////////////////////////////////////////////////////
> +// PURLClassifierLocalParent
> +
> +PURLClassifierLocalParent*
> +BackgroundParentImpl::AllocPURLClassifierLocalParent(const URIParams& aURI,
> +                                              const nsCString& aTables)

indentation

::: ipc/glue/BackgroundParentImpl.cpp:891
(Diff revision 2)
> +  return actor.forget().take();
> +}
> +
> +mozilla::ipc::IPCResult
> +BackgroundParentImpl::RecvPURLClassifierLocalConstructor(PURLClassifierLocalParent* aActor,
> +                                                  const URIParams& aURI,

indentation

::: ipc/glue/BackgroundParentImpl.cpp:909
(Diff revision 2)
> +
> +    auto* actor = static_cast<URLClassifierLocalParent*>(aActor);
> +    actor->StartClassify(uri, aTables);
> +  });
> +
> +  NS_DispatchToMainThread(r);

this can fail. Plus you should use AbstractThread and SystemGroup

::: netwerk/base/nsIURIClassifier.idl:17
(Diff revision 2)
>  
>  interface nsIChannel;
>  interface nsIEventTarget;
>  interface nsIPrincipal;
>  interface nsIURI;
> +interface nsIThread;

alphabetic order

::: toolkit/components/url-classifier/moz.build:81
(Diff revision 2)
>  ]
>  
>  FINAL_LIBRARY = 'xul'
>  
>  LOCAL_INCLUDES += [
> -    '../build',
> +    '../build'

don't change this.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:60
(Diff revision 2)
>  #include "mozilla/dom/URLClassifierChild.h"
>  #include "mozilla/ipc/URIUtils.h"
>  #include "nsProxyRelease.h"
>  #include "SBTelemetryUtils.h"
> +#include "mozilla/ipc/BackgroundChild.h"
> +#include "mozilla/ipc/PBackgroundChild.h"

alphabetic order

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1661
(Diff revision 2)
>      if (inSafeMode) {
>        return NS_ERROR_NOT_AVAILABLE;
>      }
>    }
>  
> +  NS_NewNamedThread(NS_LITERAL_CSTRING("URL Classifier IPC"),

here you are creating a new thread... why?

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1915
(Diff revision 2)
> +    BackgroundChild::GetForCurrentThread();
> +  // If it's not spun up yet, block until it is, and retry
> +  if (!existingBackgroundChild) {
> +    LOG(("No existingBackgroundChild"));
> +    existingBackgroundChild =
> +      BackgroundChild::SynchronouslyCreateForCurrentThread();

this can fail.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1924
(Diff revision 2)
> +
> +namespace {
> +
> +// This callback wrapper is required when we need to pass the
> +// possibly-non-thread-safe callback around. The destruction
> +// will be always on main thread since the original callback

this is not what you have implemented. In the DTOR you check if the current thread is not main-thread.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1971
(Diff revision 2)
>  {
>    MOZ_ASSERT(NS_IsMainThread(), "AsyncClassifyLocalWithTables must be called "
>                                  "on main thread");
>  
> +  // Wrap around the raw input for convenient use.
> +  RefPtr<nsIThread> nonNullCallbackThread =

here you should use nsIEventTarget as well.
Attachment #8857961 - Flags: review?(amarchesini) → review-
Comment on attachment 8857961 [details]
Bug 1356211 - Part 1: Move PURLClassifierLocal from PContent to PBackground. (interface changes)

https://reviewboard.mozilla.org/r/130004/#review136322

Thanks for the review baku :)

I will try to use I/O and see if that still works well as I expect.

> I need more information in order to understand this code. URLClassifier is main-thread only, if we want to run the callback on a separate thread, why we cannot just use I/O thread for them?

Yes what we want is to *conditionally* run the callback off the main thread. Let me try using I/O for them in the next patch. That is supposed to work. Thanks :)

> here you are creating a new thread... why?

This new thread is for *synchronously* createing PBackgroundChild for IPC. Would you strongly suggest to *asynchronously* create PBackgroundChild on main thread?

> this is not what you have implemented. In the DTOR you check if the current thread is not main-thread.

Oops I should have said: "The destruction of the callback will be always on the main thread....".

(However, the most precise description should be "The callback will always be Release()'ed on the main thread.")
(In reply to Andrea Marchesini [:baku] from comment #4)
> Comment on attachment 8857961 [details]
> Bug 1356211 - Move PURLClassifierLocal from PContent to PBackground and
> support custom callback thread.
> 
> https://reviewboard.mozilla.org/r/130004/#review136322
> 
> ::: commit-message-60fe2:6
> (Diff revision 2)
> > +Bug 1356211 - Move PURLClassifierLocal from PContent to PBackground and support custom callback thread.
> > +
> > +This change is for Bug 1345058, where we need the callback to occur off the main thread
> > +so that we can be waiting on main thread for the callback. In order to support custom
> > +callback thread, the IPC thread cannot be the main thread so the parent protocol of
> > +PURLClassifierLocal is changed to PBackground.
> 
> I need more information in order to understand this code. URLClassifier is
> main-thread only, if we want to run the callback on a separate thread, why
> we cannot just use I/O thread for them?
> 

Hi :baku,

Sorry I might have to clarify this a little bit: what is the I/O thread you refer here?
Does it refer to the urlclassifier worker thread or IOThreadChild [1] or any thread else?

Thanks :)

[1] http://searchfox.org/mozilla-central/source/ipc/glue/IOThreadChild.h
Flags: needinfo?(amarchesini)
Sorry I got more questions so I am going to put them altogether here:

1) The reason I didn't use I/O thread for callback.

My very first idea was to fire callback on the existed thread but then felt 
a little uncomfortable that if a callback would be called on a thread which
is neither the calling thread nor the custom thread.  

Besides, the reason I have to fire callback on non-main-thread is for Bug 1345058,
where the async function will be called in the very early stage to initialize
an important variable. However, that variable might be not ready when we need it. 
In that case, we have to wait until the callback is fired "on the main thread".
If the callback is also on the main thread, we would be stuck forever.

2) If firing callback on the custom thread is not a good idea in your opinion,
I'd like to be clear that what specific I/O thread did you referred? (comment 6)

3) The new thread I create is for *synchronously* creating PBackgroundChild. 
   (comment 5)

Thanks!
Hi :baku,

I have submitted a new patch which addressed most of the comments 
from the last review except for

1) Where to callback. 

I didn't change the way to determine where the callback should be occurred 
(still via the API). I've tried to rationalize my design in comment 7.

2) A creation of a new thread in nsUrlClassifierDBService. 

Explantation is in comment 5.

Thanks :)
I have some additional questions before finishing the review.
First of all, in this patch there are too many things. Can you split it? Maybe following my questions:

1. You are adding thre nsIThread here: nsIURIClassifier.asyncClassifyLocalWithTables so that you can specify in which thread you want the callback to run. This is great. Would be nice to have it as separate patch.

2. Why do you need to be "waiting on main thread for callback"? Tell me more about the setup you have here. In theory, the code should be activated when the callback is executed. Is this code already in m-i? Can you show me what needs this?
Flags: needinfo?(amarchesini) → needinfo?(hchang)
(In reply to Andrea Marchesini [:baku] from comment #11)
> I have some additional questions before finishing the review.
> First of all, in this patch there are too many things. Can you split it?

Sure. I'll try to split it.

> Maybe following my questions:
> 
> 1. You are adding thre nsIThread here:
> nsIURIClassifier.asyncClassifyLocalWithTables so that you can specify in
> which thread you want the callback to run. This is great. Would be nice to
> have it as separate patch.
> 
> 2. Why do you need to be "waiting on main thread for callback"? Tell me more
> about the setup you have here. In theory, the code should be activated when
> the callback is executed. Is this code already in m-i? Can you show me what
> needs this?

The actual use case of the custom callback thread is for bug 1345058.
(other than that, the main thread will be the main thread)

You can find the patch here https://reviewboard.mozilla.org/r/129274/diff/5

In short, what we originally do is to use a "sync API" to decide if a document
(based on its principal) should be blocked. In order to get rid of that sync API,
the async version API was implemented and will be used to replace the sync API
in bug 1345058.

However, it's too hard to perfectly replace the sync with the async API.
What we can do right now is try to call that async API in the very
early stage (nsDocument::StartDocumentLoad) and save the result for later use.
The result will be accessed on the main thread and chances are the result
hasn't come out when it's needed. In this case, we have to wait on the main
thread for callback.

p.s.

I don't know how to locate specific lines in the reviewboard patch
so you can search for the following key word in the patch

https://reviewboard.mozilla.org/r/129274/diff/5

PrincipalFlashClassifier::Result         ==>  where we may wait on the main thread
PrincipalFlashClassifier::AsyncClassify  ==>  where the async API is called
Flags: needinfo?(hchang)
Flags: needinfo?(amarchesini)
Attachment #8863589 - Flags: review?(amarchesini)
Attachment #8863590 - Flags: review?(amarchesini)
Thanks for comment 12. I have an additional question:

can you keep both sync and async calls? So that, when you need data, if the async haven't returned yet, you can use the sync version? With a flag, you can ignore the async response when it arrives. In this way we don't need to block the main-thread with a:

mozilla::MonitorAutoLock lock(mMonitor);
while (mClassifying) {
  lock.Wait();
}
Flags: needinfo?(amarchesini) → needinfo?(hchang)
(In reply to Andrea Marchesini [:baku] from comment #16)
> Thanks for comment 12. I have an additional question:
> 
> can you keep both sync and async calls? So that, when you need data, if the
> async haven't returned yet, you can use the sync version? With a flag, you
> can ignore the async response when it arrives. In this way we don't need to
> block the main-thread with a:
> 
> mozilla::MonitorAutoLock lock(mMonitor);
> while (mClassifying) {
>   lock.Wait();
> }

Hi :baku,

I could but the sync API is still blocking the main thread in essence.
Would that make any difference? Or you point is to avoid supporting
callback on non-main thread?

Thanks.
Flags: needinfo?(hchang)
Flags: needinfo?(amarchesini)
Per offline discussion with baku, we decide to take the approach in comment 16
so this bug is no longer needed.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Flags: needinfo?(amarchesini)
Attachment #8857961 - Flags: review?(amarchesini)
Attachment #8863589 - Flags: review?(amarchesini)
Attachment #8863590 - Flags: review?(amarchesini)
Flags: needinfo?(ajones)
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: