Support asyncClassifyLocal in content process

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Safe Browsing
P1
normal
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: hchang, Assigned: hchang)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

6 months ago
Assignee: nobody → hchang
(Assignee)

Comment 1

6 months ago
Created attachment 8842279 [details] [diff] [review]
Bug1343425.patch
(Assignee)

Updated

6 months ago
Depends on: 1341506
Status: NEW → ASSIGNED
Priority: -- → P2
Priority: P2 → P3
This will be required before we can make the flash blocking functionality asynchronous.

Updated

6 months ago
Blocks: 1342333
p1 because it supports
Whiteboard: [qf:p1]
p1 because it supports https://bugzilla.mozilla.org/show_bug.cgi?id=1331674 SyncIPC bug
(Assignee)

Updated

5 months ago
Blocks: 1345058
(Assignee)

Comment 5

5 months ago
I will be working on this issue since it blocks Bug 1345058, which is the last use of the sync urlclassifier function.
(Assignee)

Comment 6

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

Comment 8

5 months ago
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
Flags: needinfo?(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
Flags: needinfo?(tnguyen)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8854832 - Flags: review?(ehsan)
(Assignee)

Updated

5 months ago
Priority: P3 → P5
(Assignee)

Updated

5 months ago
Priority: P5 → P1
(Assignee)

Comment 11

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

Updated

5 months ago
Attachment #8842279 - Attachment is obsolete: true
Attachment #8854832 - Flags: review?(ehsan) → review?(amarchesini)
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);
Attachment #8854832 - Flags: review?(amarchesini) → review+
Comment hidden (mozreview-request)

Comment 14

5 months ago
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63d2e75ee675
Supports nsIURIClassifier.asyncClassifyLocalWithTables. r=baku
https://hg.mozilla.org/mozilla-central/rev/63d2e75ee675
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
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)
(Assignee)

Updated

4 months ago
Attachment #8854832 - Flags: review?(ehsan)
You need to log in before you can comment on or make changes to this bug.