1318768 - Make nsIURIClassifier usable in the content process

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[(no longer active)]

The URL classifier service requires access to the profile directory, so currently it cannot be used from within the content process.  I have some patches to make it work in e10s by forwarding the requests to the parent process.

Comment 1

[(no longer active)]

Attachment: Part 1: Make the URL classifier service initialize in the content process

Comment 2

[(no longer active)]

Attachment: Part 2: Make nsIURIClassifier::Classify() available in the content process

Comment 3

[(no longer active)]

Attachment: Part 3: Make nsIURIClassifier::ClassifyLocalWithTables() usable in the content process

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8812371 [details] [diff] [review]
Part 1: Make the URL classifier service initialize in the content process

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

There's a few calls in the Init function that require being on the main thread. Might be a good idea to verify that the main thread in a content process is "good enough".

Comment 5

[(no longer active)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #4)
> Comment on attachment 8812371 [details] [diff] [review]
> Part 1: Make the URL classifier service initialize in the content process
> 
> Review of attachment 8812371 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's a few calls in the Init function that require being on the main
> thread. Might be a good idea to verify that the main thread in a content
> process is "good enough".

We're skipping almost all of Init() though.  I'm not quite sure what you mean?

Comment 6

[Gian-Carlo Pascutto [:gcp]]

(In reply to :Ehsan Akhgari from comment #5)
> We're skipping almost all of Init() though.

Ah right. All good then.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8812372 [details] [diff] [review]
Part 2: Make nsIURIClassifier::Classify() available in the content process

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

I don't think I'm qualified to review the dom/ipc changes.

::: dom/ipc/URLClassifierParent.cpp
@@ +26,5 @@
> +  nsresult rv = uriClassifier->Classify(aPrincipal, aUseTrackingProtection,
> +                                        this, aSuccess);
> +  if (NS_FAILED(rv) || !*aSuccess) {
> +    // If we fail to classify, we're probably dealing with a non-HTTP(S) URI,
> +    // so just pretend that we succeeded.

Are you sure? Classify does reset NS_ERROR_MALFORMED_URI into aSuccess = false (same for nothing being enabled), so that's OK. But I don't see that NS_FAILED(rv) would necessarily be benign.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8812373 [details] [diff] [review]
Part 3: Make nsIURIClassifier::ClassifyLocalWithTables() usable in the content process

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

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1484,5 @@
> +    }
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  PROFILER_LABEL_FUNC(js::ProfileEntry::Category::OTHER);

What's the idea here? Do we want this in other places too?

Comment 9

[(no longer active)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #7)
> Comment on attachment 8812372 [details] [diff] [review]
> Part 2: Make nsIURIClassifier::Classify() available in the content process
> 
> Review of attachment 8812372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think I'm qualified to review the dom/ipc changes.
> 
> ::: dom/ipc/URLClassifierParent.cpp
> @@ +26,5 @@
> > +  nsresult rv = uriClassifier->Classify(aPrincipal, aUseTrackingProtection,
> > +                                        this, aSuccess);
> > +  if (NS_FAILED(rv) || !*aSuccess) {
> > +    // If we fail to classify, we're probably dealing with a non-HTTP(S) URI,
> > +    // so just pretend that we succeeded.
> 
> Are you sure? Classify does reset NS_ERROR_MALFORMED_URI into aSuccess =
> false (same for nothing being enabled), so that's OK. But I don't see that
> NS_FAILED(rv) would necessarily be benign.

During testing I found out that at least in the case of a system principal being passed in, <searchfox.org/mozilla-central/rev/904bf9addd03b03d4cad11b82f19f43d875b7f27/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1487> returns an error since the URI will be null, so I kind of assumed that a failure error code also means the same thing as *aSuccess being false.

What should I do here?  Should I return both a boolean and an nsresult?

Comment 10

[(no longer active)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)
> Comment on attachment 8812373 [details] [diff] [review]
> Part 3: Make nsIURIClassifier::ClassifyLocalWithTables() usable in the
> content process
> 
> Review of attachment 8812373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
> @@ +1484,5 @@
> > +    }
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  PROFILER_LABEL_FUNC(js::ProfileEntry::Category::OTHER);
> 
> What's the idea here? Do we want this in other places too?

I'm just moving this line to after the |if (XRE_IsContentProcess())| block.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

(In reply to :Ehsan Akhgari from comment #10)
> > > +  PROFILER_LABEL_FUNC(js::ProfileEntry::Category::OTHER);
> > 
> > What's the idea here? Do we want this in other places too?
> 
> I'm just moving this line to after the |if (XRE_IsContentProcess())| block.

Oops, missed that. Thought it was a new line.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

(In reply to :Ehsan Akhgari from comment #9)
> During testing I found out that at least in the case of a system principal
> being passed in,
> <searchfox.org/mozilla-central/rev/904bf9addd03b03d4cad11b82f19f43d875b7f27/
> toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1487> returns
> an error since the URI will be null, so I kind of assumed that a failure
> error code also means the same thing as *aSuccess being false.
> 
> What should I do here?  Should I return both a boolean and an nsresult?

I'd normally have a slightly preference to not change the behavior, especially in these kind of edge cases where it's not clear how exactly this can fail. 

On the other hand, the callers that you will be having will all be new code, right? So they can't be acting on the distinction here. So just keep the code as is and maybe clarify the comment a little.

Comment 13

[(no longer active)]

OK, sounds good!

Comment 14

[(no longer active)]

Attachment: Part 2: Make nsIURIClassifier::Classify() available in the content process

baku, can you please review the dom parts?  Thanks!

Comment 15

[(no longer active)]

Attachment: Part 3: Make nsIURIClassifier::ClassifyLocalWithTables() usable in the content process

baku, can you please review the dom parts?  Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b23dc33ac32
Part 1: Make the URL classifier service initialize in the content process; r=gcp
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80a175c7f3f
Part 2: Make nsIURIClassifier::Classify() available in the content process; r=gcp,baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb3e4ebc788
Part 3: Make nsIURIClassifier::ClassifyLocalWithTables() usable in the content process; r=gcp,baku

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4b23dc33ac32
https://hg.mozilla.org/mozilla-central/rev/f80a175c7f3f
https://hg.mozilla.org/mozilla-central/rev/3cb3e4ebc788

Comment 18

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8812371 [details] [diff] [review]
Part 1: Make the URL classifier service initialize in the content process


Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Can't run the study as intended
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not for this feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected
[List of other uplifts needed for the feature/fix]: Bug 1318768, Bug 1323220, Bug 1325255, Bug 1322204, Bug 1325651, Bug 1319571, Bug 1321377, Bug 1307604, Bug 1323064, Bug 1335549, Bug 1333303, Bug 1333483, Bug 1336714, Bug 1338287
[Is the change risky?]: Somewhat risky.
[Why is the change risky/not risky?]: The code is generic, and the consumers are not well known/studied. There have been a number of crashes and regressions as a result of this code. Fixes have had uplift requested as well.
[String changes made/needed]: none

Comment 19

[Robin Steuber (they/them) [:bytesized]]

Attachment: Part 2: Make nsIURIClassifier::Classify() available in the content process (beta uplift)

Comment 20

[Robin Steuber (they/them) [:bytesized]]

Attachment: Part 3: Make nsIURIClassifier::ClassifyLocalWithTables() usable in the content process (beta uplift)

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8812371 [details] [diff] [review]
Part 1: Make the URL classifier service initialize in the content process

It's been in Aurora53 already. Aurora53-.

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8812371 [details] [diff] [review]
Part 1: Make the URL classifier service initialize in the content process

this was deemed too risky for beta