Make nsIURIClassifier usable in the content process

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 affected, firefox53 fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8812371 [details] [diff] [review]
Part 1: Make the URL classifier service initialize in the content process
Attachment #8812371 - Flags: review?(gpascutto)
(Assignee)

Comment 2

2 years ago
Created attachment 8812372 [details] [diff] [review]
Part 2: Make nsIURIClassifier::Classify() available in the content process
Attachment #8812372 - Flags: review?(gpascutto)
(Assignee)

Comment 3

2 years ago
Created attachment 8812373 [details] [diff] [review]
Part 3: Make nsIURIClassifier::ClassifyLocalWithTables() usable in the content process
Attachment #8812373 - Flags: review?(gpascutto)
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+
(Assignee)

Comment 5

2 years ago
(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+
(Assignee)

Comment 9

2 years ago
(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)
(Assignee)

Comment 10

2 years ago
(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)
(Assignee)

Comment 13

2 years ago
OK, sounds good!
(Assignee)

Comment 14

2 years ago
Created attachment 8812962 [details] [diff] [review]
Part 2: Make nsIURIClassifier::Classify() available in the content process

baku, can you please review the dom parts?  Thanks!
Attachment #8812962 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8812372 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
Created attachment 8812963 [details] [diff] [review]
Part 3: Make nsIURIClassifier::ClassifyLocalWithTables() usable in the content process

baku, can you please review the dom parts?  Thanks!
Attachment #8812963 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8812373 - Attachment is obsolete: true
Attachment #8812962 - Flags: review?(amarchesini) → review+
Attachment #8812963 - Flags: review?(amarchesini) → review+

Comment 16

2 years ago
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

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b23dc33ac32
https://hg.mozilla.org/mozilla-central/rev/f80a175c7f3f
https://hg.mozilla.org/mozilla-central/rev/3cb3e4ebc788
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Updated

2 years ago
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?
Created attachment 8837396 [details] [diff] [review]
Part 2: Make nsIURIClassifier::Classify() available in the content process (beta uplift)
Attachment #8837396 - Flags: approval-mozilla-beta?
Created attachment 8837397 [details] [diff] [review]
Part 3: Make nsIURIClassifier::ClassifyLocalWithTables() usable in the content process (beta uplift)
Attachment #8837397 - Flags: approval-mozilla-beta?

Updated

2 years ago
status-firefox52: --- → affected
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-

Updated

2 years ago
Attachment #8812962 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-

Updated

2 years ago
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.