Closed
Bug 1343425
Opened 8 years ago
Closed 8 years ago
Support asyncClassifyLocal in content process
Categories
(Toolkit :: Safe Browsing, enhancement, P1)
Toolkit
Safe Browsing
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hchang, Assigned: hchang)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-review-board-request
|
Details |
No description provided.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → hchang
Assignee | ||
Comment 1•8 years ago
|
||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Updated•8 years ago
|
Priority: P2 → P3
Comment 2•8 years ago
|
||
This will be required before we can make the flash blocking functionality asynchronous.
Comment 4•8 years ago
|
||
p1 because it supports https://bugzilla.mozilla.org/show_bug.cgi?id=1331674 SyncIPC bug
Assignee | ||
Comment 5•8 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•8 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•8 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•8 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•8 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•8 years ago
|
Attachment #8854832 -
Flags: review?(ehsan)
Assignee | ||
Updated•8 years ago
|
Priority: P3 → P5
Assignee | ||
Updated•8 years ago
|
Priority: P5 → P1
Assignee | ||
Comment 11•8 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•8 years ago
|
Attachment #8842279 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8854832 -
Flags: review?(ehsan) → review?(amarchesini)
Comment 12•8 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•8 years ago
|
||
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63d2e75ee675
Supports nsIURIClassifier.asyncClassifyLocalWithTables. r=baku
Comment 15•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 16•8 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•8 years ago
|
Attachment #8854832 -
Flags: review?(ehsan)
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•