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

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?

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

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?

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

I haven't seen anything, no.

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

(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?

MozReview-Commit-ID: Ho5IE62lYRl

MozReview-Commit-ID: tWd6gG14Ty

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

(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!

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

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.

(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.

(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

(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.

MozReview-Commit-ID: HfUNtkBwscr

MozReview-Commit-ID: IGn1ILHN4PI

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 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 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...

(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.

MozReview-Commit-ID: BztjVMlQngi

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+

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

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

(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!