1339006 - DocGroup labeling in toolkit/components/url-classifier

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Ethan Tseng [:ethan]]

This bug is to apply labeling APIs (https://wiki.mozilla.org/Quantum/DOM) to the Safe Browsing component implementation.

Comment 1

[François Marier [:francois]]

Ehsan, the only content process work we do is what you added in bug 1318768.

Is that already labelled correctly or do we need to do a follow up patch?

Comment 2

[(no longer active)]

No, it wasn't labeled since it landed before the labeling APIs landed.

Comment 3

[Thomas Nguyen (:tnguyen)]

afaik, only content process runnables need to be labeled.
I am scanning at url-classifier to see if we have any case using runnable in content process.

- nsIURIClassifier : classify, classifyLocalWithTables, classifyLocal
These could run on content process, but in content process, we simply forward all requests to the parent process. All the classify things are run on parent process. Dont need to label runnable

- SafeBrowsing.jsm (and listmanager.js) This file will control DB updating and should only be inited in parent process. Updating related runnables should be run on parent process, no need to label them

- Let see nsUrlClassifierDBService::Lookup in [1]
[1] http://searchfox.org/mozilla-central/source/toolkit/components/downloads/ApplicationReputation.cpp#339
But this is expected to run on main process. (ApplicationReputation api will be called on parent process from DownloadIntergration.jsm)

Basically, runnables in url-classifier dont require to be labeled, or am I missing anything?

Comment 4

[(no longer active)]

Bill, is anything from this code showing up in your studies of the unlabeled runnables?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I haven't seen anything, no.

Comment 6

[Thomas Nguyen (:tnguyen)]

Continue scanning child actors created by child IPC request and TimerCallback, there's only 1 place we need to label in url-classifier
http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1487

Comment 7

[Thomas Nguyen (:tnguyen)]

(In reply to Bill McCloskey (:billm) from comment #5)
> I haven't seen anything, no.

That said, nsIURIClassifier.classify [1] is supposed to be able to work in content process but all of its usages are in parent process at the moment [2] and [3] (and test case in [4])
[1] http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1476
[2] https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/dom/ipc/URLClassifierParent.cpp#28
[3] https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/netwerk/base/nsChannelClassifier.cpp#412
[4] https://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/toolkit/components/url-classifier/tests/mochitest/test_classifier.html#143
[5] http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1487
The actor constructor in [5] required to be labeled, but the given nsIUri or nsIPrincipal is not enough to do labelling. We may have to pass a more useful argument such as nsIChannel, or directly give a nsIEventTarget. I prefer passing nsIEventTarget and adding an assertion.
Hi Ehsan, could you please tell any case of using classify in content process as you added bug 1318768? Could I change nsIPrincipal* aPrincipal to nsIChannel in your case?

Comment 8

[Thomas Nguyen (:tnguyen)]

Attachment: Specify event target if we run nsIURIClassifier.clasify() from content process

MozReview-Commit-ID: Ho5IE62lYRl

Comment 9

[Thomas Nguyen (:tnguyen)]

Attachment: Specify event target if we run nsIURIClassifier.clasify() from content process

MozReview-Commit-ID: tWd6gG14Ty

Comment 10

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8843247 [details] [diff] [review]
Specify event target if we run nsIURIClassifier.clasify() from content process

Review of attachment 8843247 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1636,5 @@
> +        return NS_ERROR_FAILURE;
> +      }
> +      nsCOMPtr<nsIEventTarget> systemGroupEventTarget
> +        = mozilla::SystemGroup::EventTargetFor(mozilla::TaskCategory::Other);
> +      content->SetEventTargetForActor(actor, systemGroupEventTarget);

Running ./mach mochitest toolkit/components/url-classifier/tests/mochitest/test_classifier_match.html will crash at
Assertion failure: !cx->enableAccessValidation || cx->compartment()->isAccessValid(), at /Volumes/B2G/Projects/mozilla-central/mozilla-central/js/src/vm/Interpreter.cpp:356

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #10)
> Assertion failure: !cx->enableAccessValidation ||
> cx->compartment()->isAccessValid(), at
> /Volumes/B2G/Projects/mozilla-central/mozilla-central/js/src/vm/Interpreter.
> cpp:356

The assertion is intended according to bug 1337537 comment 4.
In your case, the js script runs nsIURIClassifier within a specific DocGroup.
However, the JS callback will be run within the SystemGroup you specified in comment 9 if the aEventTarget was not provided from js.

Here comes a problem:
1. It's possible to access XPC service from web content in mochitest.
2. However, as I know, there is no way to provide a DocGroup-EventTarget from web content scripts.

Bill,

Shall we just keep the event target of this actor child unspecified in this use case instead of specifying a SystemGroup to it since this won't impact the scheduler in real web contents? Is there any better suggestion?

Thanks!

Comment 12

[(no longer active)]

I'm pretty sure you should use the SystemGroup for this one.  AFAIK this won't be directly tied to any web page script.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The way I've been handling these test issues is as follows: Add a method to SpecialPowers that calls classify. It would wrap the callback that's passed in with something that dispatches to the main thread. Here's an example for observers:
http://searchfox.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#1216-1242

This works because the SpecialPowers code is chrome-privileged, so it's allowed to run inside the SystemGroup. Then the runnable it dispatches is unlabeled, so it can do anything.

Comment 14

[Thomas Nguyen (:tnguyen)]

(In reply to :Ehsan Akhgari from comment #12)
> I'm pretty sure you should use the SystemGroup for this one.  AFAIK this
> won't be directly tied to any web page script.

Thanks Ehsan, that would simpler to use SystemGroup, but I am only still a bit concerned here. 
Apparently the classify wont be directly tied to any web page script, but the classify is async call then a lot of things will be blocked until we receive classify callback (a channel openning will be suspended until OnClassifyComplete is called)
As I discussed with Bevis, if we label X as a SystemGroup runnable, mean the X could be run whatever we want. We would put X behind foreground tabs runnalble in queue, (or even behind background, I don't see the real code of scheduling model contains SystemGroup runnable now, that seems not be specified at the moment), for example F1,F2,F3, B1,B2,B3, X.
Assume that X is ipc runnables in classify there, I doubt that the callback will be late for a while due to scheduling.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #14)
> (In reply to :Ehsan Akhgari from comment #12)
> > I'm pretty sure you should use the SystemGroup for this one.  AFAIK this
> > won't be directly tied to any web page script.
> 
> Thanks Ehsan, that would simpler to use SystemGroup, but I am only still a
> bit concerned here. 
> Apparently the classify wont be directly tied to any web page script, but
> the classify is async call then a lot of things will be blocked until we
> receive classify callback (a channel openning will be suspended until
> OnClassifyComplete is called)
IMO, if the procedure blocked by this onClassifyComplete callback is related to a web content, then it is important to set the actor with a proper EventTarget to ensure that this callback will be run in proper priority other than SystemGroup priority.
> As I discussed with Bevis, if we label X as a SystemGroup runnable, mean the
> X could be run whatever we want. We would put X behind foreground tabs
> runnalble in queue, (or even behind background, I don't see the real code of
> scheduling model contains SystemGroup runnable now, that seems not be
> specified at the moment), for example F1,F2,F3, B1,B2,B3, X.
> Assume that X is ipc runnables in classify there, I doubt that the callback
> will be late for a while due to scheduling.
This is referred to the last few sentences about SystemGroup Runnables explained in the following question:
https://wiki.mozilla.org/Quantum/DOM#Q1:_What_is_the_impact_of_leaving_a_runnable_unlabeled.3F

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bill McCloskey (:billm) from comment #13)
> The way I've been handling these test issues is as follows: Add a method to
> SpecialPowers that calls classify. It would wrap the callback that's passed
> in with something that dispatches to the main thread. Here's an example for
> observers:
> http://searchfox.org/mozilla-central/source/testing/specialpowers/content/
> specialpowersAPI.js#1216-1242
> 
> This works because the SpecialPowers code is chrome-privileged, so it's
> allowed to run inside the SystemGroup. Then the runnable it dispatches is
> unlabeled, so it can do anything.
m.. the thing different from keeping it unlabeled is that, in mochitest, if there is any callback from a xpcom service, the callback will be wrapped with *additional runnable* internally by SpecialPower.
I am neutral to either keep it unlabeled or having this additional wrapper in this use case, but if setting to SystemGroup ensures that no potential issue of having runnable unlabeled in the future, then it's a good option.

Comment 17

[Thomas Nguyen (:tnguyen)]

Attachment: Specify event target if we run nsIURIClassifier.clasify() from content process

MozReview-Commit-ID: HfUNtkBwscr

Comment 18

[Thomas Nguyen (:tnguyen)]

Attachment: Specify event target if we run nsIURIClassifier.clasify() from content process

MozReview-Commit-ID: IGn1ILHN4PI

Comment 19

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8843894 [details] [diff] [review]
Specify event target if we run nsIURIClassifier.clasify() from content process

Hi Ehsan, gcp.
Could you please take a look at the patch?
Basically, I passed a nsIEventTarget to nsIURIClassifier.classify which should be tied to DocGroup and TabGroup to do labelling. A fallback will use SystemGroup in the case we have null nsIEventTarget (mochitest for example)

Comment 20

[(no longer active)]

Comment on attachment 8843894 [details] [diff] [review]
Specify event target if we run nsIURIClassifier.clasify() from content process

Review of attachment 8843894 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great to me, thanks!

Comment 21

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8843894 [details] [diff] [review]
Specify event target if we run nsIURIClassifier.clasify() from content process

Review of attachment 8843894 [details] [diff] [review]:
-----------------------------------------------------------------

I'll defer to billm and ehsan here. (Having to special case the mochitests does raise an eyebrow for me, different codepaths between tests and actual usage tends to backfire)

::: testing/specialpowers/content/specialpowersAPI.js
@@ +2120,5 @@
>    },
> +
> +  /* Bug 1339006 Runnables of nsIURIClassifier.classify may be labeled by
> +   * SystemGroup, but some test cases may run as web content. That would assert
> +   * when trying to enter web content from a runnabled labeled by the

typo: ...from a runnable...

Comment 22

[(no longer active)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #21)
> I'll defer to billm and ehsan here. (Having to special case the mochitests
> does raise an eyebrow for me, different codepaths between tests and actual
> usage tends to backfire)

There is actually no difference here after the patch.  Before the patch, the APIs in question are callable from Web content due to the fact that the tests call them directly, and the test changes are just fixing things so that it doesn't happen in the first place.

Comment 23

[Thomas Nguyen (:tnguyen)]

Attachment: Specify event target if we run nsIURIClassifier.clasify() from content process

MozReview-Commit-ID: BztjVMlQngi

Comment 24

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8844731 [details] [diff] [review]
Specify event target if we run nsIURIClassifier.clasify() from content process

Thanks Ehsan, gcp for your review.
Carry r+

Comment 25

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c55f588d83b46fcfb7547f943fa5f66be2a6b614

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thank you Thomas for taking care of this so quickly. :)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95981ff95b20
Specify event target if we run nsIURIClassifier.clasify() from content process.r=ehsan, r=gcp

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95981ff95b20

Comment 29

[Ethan Tseng [:ethan]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #26)
> Thank you Thomas for taking care of this so quickly. :)

Yes! Good job, Thomas. Thanks!