1343425 - Support asyncClassifyLocal in content process

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement, P1)

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug1343425.patch

Comment 2

[François Marier [:francois]]

This will be required before we can make the flash blocking functionality asynchronous.

Comment 3

[Naveed Ihsanullah [:naveed]]

p1 because it supports

Comment 4

[Naveed Ihsanullah [:naveed]]

p1 because it supports https://bugzilla.mozilla.org/show_bug.cgi?id=1331674 SyncIPC bug

Comment 5

[Henry Chang [:hchang]]

I will be working on this issue since it blocks Bug 1345058, which is the last use of the sync urlclassifier function.

Comment 6

[Henry Chang [:hchang]]

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!

Comment 7

[Thomas Nguyen (:tnguyen)]

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.

Comment 8

[Henry Chang [:hchang]]

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?

http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/testing/specialpowers/content/specialpowersAPI.js#2129

Comment 9

[Thomas Nguyen (:tnguyen)]

What we see here is that the API is called from Web Content because the tests call them directly. Then http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/testing/specialpowers/content/specialpowersAPI.js#2129
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.
Thanks

Comment 10

[Henry Chang [:hchang]]

Attachment: Bug 1343425 - Supports nsIURIClassifier.asyncClassifyLocalWithTables.

We add a new "on-off" protocol PURLClassifierLocal which calls
nsIURIClassifier.asyncClassifyLocalWithTables on construction then
does callback on destruction. Pretty much the same design as PURLClassifier.

In order to avoid code duplicate, the actor implementation is templatized
and |MaybeInfo| in PURLClassifier.ipdl is moved around.

Test case is included but the custom event target is not in place for labelling.
The custom event target will be done in Bug 1353701.

Review commit: https://reviewboard.mozilla.org/r/126784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126784/

Comment 11

[Henry Chang [:hchang]]

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.

Thanks.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8854832 [details]
Bug 1343425 - Supports nsIURIClassifier.asyncClassifyLocalWithTables.

https://reviewboard.mozilla.org/r/126784/#review129830

::: 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) {

NS_WARN_IF

::: 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;

MOZ_ASSERT(aURI);

Comment 13

[Henry Chang [:hchang]]

Comment on attachment 8854832 [details]
Bug 1343425 - Supports nsIURIClassifier.asyncClassifyLocalWithTables.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126784/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63d2e75ee675
Supports nsIURIClassifier.asyncClassifyLocalWithTables. r=baku

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/63d2e75ee675

Comment 16

[(no longer active)]

So sorry for my lag here, and thanks for working on this!  I'm glad this didn't end up remaining blocked on me.  :-)