Closed Bug 1343425 Opened 3 years ago Closed 3 years ago

Support asyncClassifyLocal in content process


(Toolkit :: Safe Browsing, enhancement, P1)




Tracking Status
firefox55 --- fixed


(Reporter: hchang, Assigned: hchang)



(Whiteboard: [qf:p1])


(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → hchang
Attached patch Bug1343425.patch (obsolete) — Splinter Review
Depends on: 1341506
Priority: -- → P2
Priority: P2 → P3
This will be required before we can make the flash blocking functionality asynchronous.
Blocks: 1342333
p1 because it supports
Whiteboard: [qf:p1]
Blocks: 1345058
I will be working on this issue since it blocks Bug 1345058, which is the last use of the sync urlclassifier function.
Hi Ehsan, 

Are you available to be my reviewer of the patch? (not submitted yet).

If yes, I'd like to know your opinion how I should address this issue at this point:

1) Like my attached WIP patch does: Extend PURLClassifier.ipdl and let DBService hold 
   a persistent PURLClassifierChild unitl DBService dies. I remember you said this requires
   much more effort so we should do it when we have a good reason to do it.

2) Still extend PURLClassifier.ipdl but leave it remain a "one-shot" protocol for 
   the newly added async function. I also had a WIP patch for this approach 
   if you can recall.

3) Add a new similar protocol, say PURLClassifierLocal.ipdl, to PContent and re-do
   what PURLClassifier.ipdl does.

As a potential reviewer, what approach would you like me to take for now?
(1) is the most ideal solution but maybe (3) is a compromised one. I really
appreciate your suggestion. Thanks!
Flags: needinfo?(ehsan)
In content process, the PURLClassifierChild actor should be tied to a docgroup or tabgroup to do labelling. If you use only 1 persistent actor, you probably have to update the eventtarget of this actor frequently. I think that's not a good idea to reset the eventtarget of actor.
Hi Thomas,

As far as I can tell, nsUrlClassifierDBService::Classify will always be given nullptr as aEventTarget, 
which leads to using SystemGroup::EventTargetFor(). Even though we have SpecialPowers.doUrlClassify(),
no one is passing non-null eventTarget to it. Did I miss anything?
Flags: needinfo?(tnguyen)
What we see here is that the API is called from Web Content because the tests call them directly. Then
is just fixing things

(In reply to Henry Chang [:henry][:hchang] from comment #8)
> which leads to using SystemGroup::EventTargetFor(). Even though we have
> SpecialPowers.doUrlClassify(),
> no one is passing non-null eventTarget to it. Did I miss anything?

Yep, you are right, Classify API is not used in content process at the moment, so you will not find any non-null eventTarget is passed to this API. But we expect a vaild eventtarget will be passed if someone would use this api in content process later.

But in your case, you will support asyncClassifyLocal exactly in content process, so I think we should take into account putting a valid eventtarget.
Flags: needinfo?(tnguyen)
Attachment #8854832 - Flags: review?(ehsan)
Priority: P3 → P5
Priority: P5 → P1
Hi Ehsan,

I submitted to mozreview and asked you for review at the moment.
Please feel free to drop or forward the review if you too busy to do it.

The persistent PURLClassifierChild doesn't seem to work
as Thomas mentioned in comment 7 so I took a sort of mixed approach 
of (2) and (3): I added a new "on-off" protocol (PURLClassifierLocal) 
for asyncClassifyLocalWithTables and reused the code 
as much as possible.

Attachment #8842279 - Attachment is obsolete: true
Attachment #8854832 - Flags: review?(ehsan) → review?(amarchesini)
Comment on attachment 8854832 [details]
Bug 1343425 - Supports nsIURIClassifier.asyncClassifyLocalWithTables.

::: dom/ipc/ContentChild.h:623
(Diff revision 1)
>    virtual bool
>    DeallocPURLClassifierChild(PURLClassifierChild* aActor) override;
> +  // PURLClassifierLocalChild
> +  virtual PURLClassifierLocalChild*
> +  AllocPURLClassifierLocalChild(const URIParams& uri,

aURI, aTables

::: dom/ipc/ContentChild.cpp:3144
(Diff revision 1)
>    delete aActor;
>    return true;
>  }
> +PURLClassifierLocalChild*
> +ContentChild::AllocPURLClassifierLocalChild(const URIParams& uri,

aURI, aTables

::: dom/ipc/ContentParent.cpp:5214
(Diff revision 1)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aActor);
> +
> +  nsCOMPtr<nsIURI> uri = DeserializeURI(aURI);
> +  if (!uri) {


::: dom/ipc/URLClassifierParent.h:29
(Diff revision 1)
> +                              const nsACString& aList,
> +                              const nsACString& aProvider,
> +                              const nsACString& aPrefix)
> +  {
> +    if (mIPCOpen) {
> +      ClassifierInfo info;

ClassifierInfo info(nsCString(aList), nsCString(aProvider), nsCString(aPrefix));

This avoid to forget any field.

::: dom/ipc/URLClassifierParent.cpp:61
(Diff revision 1)
> +NS_IMPL_ISUPPORTS(URLClassifierLocalParent, nsIURIClassifierCallback)
> +
> +mozilla::ipc::IPCResult
> +URLClassifierLocalParent::StartClassify(nsIURI* aURI, const nsACString& aTables)
>  {
> -  if (mIPCOpen) {
> +  nsresult rv = NS_OK;

Attachment #8854832 - Flags: review?(amarchesini) → review+
Pushed by
Supports nsIURIClassifier.asyncClassifyLocalWithTables. r=baku
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
So sorry for my lag here, and thanks for working on this!  I'm glad this didn't end up remaining blocked on me.  :-)
Flags: needinfo?(ehsan)
Attachment #8854832 - Flags: review?(ehsan)
You need to log in before you can comment on or make changes to this bug.