Support asyncClassifyLocal in content process

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hchang, Assigned: hchang)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → hchang
(Assignee)

Updated

2 years 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.
Blocks: 1342333
p1 because it supports
Whiteboard: [qf:p1]
(Assignee)

Updated

2 years ago
Blocks: 1345058
(Assignee)

Comment 5

2 years 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

2 years 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)

Comment 7

2 years ago
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

2 years 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)

Comment 9

2 years ago
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

2 years ago
Attachment #8854832 - Flags: review?(ehsan)
(Assignee)

Updated

2 years ago
Priority: P3 → P5
(Assignee)

Updated

2 years ago
Priority: P5 → P1
(Assignee)

Comment 11

2 years 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

2 years ago
Attachment #8842279 - Attachment is obsolete: true
Attachment #8854832 - Flags: review?(ehsan) → review?(amarchesini)

Comment 12

2 years ago
mozreview-review
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

2 years 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: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 16

2 years ago
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

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