Closed Bug 1318768 Opened 3 years ago Closed 3 years ago

Make nsIURIClassifier usable in the content process

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- affected
firefox53 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(5 files, 2 obsolete files)

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 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".
Attachment #8812371 - Flags: review?(gpascutto) → review+
(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?
(In reply to :Ehsan Akhgari from comment #5)
> We're skipping almost all of Init() though.

Ah right. All good then.
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.
Attachment #8812372 - Flags: review?(gpascutto) → review+
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?
Attachment #8812373 - Flags: review?(gpascutto) → review+
(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?
Flags: needinfo?(gpascutto)
(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.
(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.
(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.
Flags: needinfo?(gpascutto)
OK, sounds good!
baku, can you please review the dom parts?  Thanks!
Attachment #8812962 - Flags: review?(amarchesini)
Attachment #8812372 - Attachment is obsolete: true
baku, can you please review the dom parts?  Thanks!
Attachment #8812963 - Flags: review?(amarchesini)
Attachment #8812373 - Attachment is obsolete: true
Attachment #8812962 - Flags: review?(amarchesini) → review+
Attachment #8812963 - Flags: review?(amarchesini) → review+
Pushed by eakhgari@mozilla.com:
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
Depends on: 1325651
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
Attachment #8812371 - Flags: approval-mozilla-beta?
Attachment #8812371 - Flags: approval-mozilla-aurora?
Attachment #8812962 - Flags: approval-mozilla-beta?
Attachment #8812962 - Flags: approval-mozilla-aurora?
Attachment #8812963 - Flags: approval-mozilla-beta?
Attachment #8812963 - Flags: approval-mozilla-aurora?
Attachment #8812962 - Flags: approval-mozilla-beta?
Attachment #8812962 - Flags: approval-mozilla-aurora?
Attachment #8812963 - Flags: approval-mozilla-beta?
Attachment #8812962 - Flags: approval-mozilla-aurora?
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-.
Attachment #8812371 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8812962 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #8812963 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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
Attachment #8812371 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8837396 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8837397 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.