Closed Bug 930456 Opened 6 years ago Closed 5 years ago

[e10s] search service initialization should explicitly fail in child processes

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan
Tracking Status
e10s later ---

People

(Reporter: Gavin, Assigned: chrishood, Mentored)

References

Details

(Whiteboard: [good first bug])

Attachments

(1 file, 9 obsolete files)

Bug 927132 suggests that it's currently failing anyhow for some reason, but I think it would be better to make that explicit. At least until we have proper search service remoting, to avoid search stuff getting corrupted/interacting poorly.
We could also skip directly to "proper search service remoting", it may not be too hard to do something like we did for content prefs:

http://hg.mozilla.org/mozilla-central/annotate/19fd3388c372/toolkit/components/contentprefs/nsContentPrefService.js#l14
Whiteboard: p=0
Attached patch manifest-filtering (obsolete) — Splinter Review
Gavin asked for something like this in https://bugzilla.mozilla.org/show_bug.cgi?id=966467#c6.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8379893 - Flags: review?(benjamin)
Comment on attachment 8379893 [details] [diff] [review]
manifest-filtering

Needs tests. Should be able to do all this using xpcshell testing; xpcshell tests can launch child processes.
Attachment #8379893 - Flags: review?(benjamin) → review-
Depends on: 930243
Summary: search service initialization should explicitly fail in child processes → [e10s] search service initialization should explicitly fail in child processes
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=1
Blocks: 1045928
Attached patch manifest-filtering v2 (obsolete) — Splinter Review
I think I can finally use this for something.
Attachment #8379893 - Attachment is obsolete: true
Attachment #8496257 - Flags: review?(benjamin)
Looks like that patch belongs on bug 930243?
Flags: needinfo?(wmccloskey)
Comment on attachment 8496257 [details] [diff] [review]
manifest-filtering v2

OK, I'll move it.
Attachment #8496257 - Attachment is obsolete: true
Attachment #8496257 - Flags: review?(benjamin)
Flags: needinfo?(wmccloskey)
This should be pretty straightforward now that we have bug 930243.
Assignee: wmccloskey → nobody
Mentor: gavin.sharp
Points: --- → 1
Whiteboard: p=1 → [good first bug]
I'd be interested in having a look into this, It looks like I need to make a call to Services.appinfo.processType and check to make sure it's not equal to a child process.  

What code file should I put this in and what do I need to do in order to ensure that this change is working properly before I write an integration test for it?
ni to answer comment 8
Flags: needinfo?(gavin.sharp)
(In reply to Chris from comment #8)
> I'd be interested in having a look into this, It looks like I need to make a
> call to Services.appinfo.processType and check to make sure it's not equal
> to a child process.  
> 
> What code file should I put this in and what do I need to do in order to
> ensure that this change is working properly before I write an integration
> test for it?

Hi Chris! Thanks for looking into it. Now that we have the functionality in bug 930243, this should be even easier than that. You should be able to just add "process=main" to the component registration line at:

https://hg.mozilla.org/mozilla-central/annotate/b915a50bc6be/toolkit/components/search/toolkitsearch.manifest#l1

For testing, you could do a manual test in e10s mode (e.g. ensure that attempting to invoke the search service in a content script, like https://hg.mozilla.org/mozilla-central/annotate/b915a50bc6be/browser/base/content/content.js, fails), and then also create an automated test very similar to the one in bug 930243's patch (xpcom/tests/unit/test_process_directives.js, test_process_directives_child.js).

Let me know if it's useful for me to elaborate further!
Flags: needinfo?(gavin.sharp)
Thanks, I'll update with a patch when I'm ready for a review.
Attached patch bug-903456.patch (obsolete) — Splinter Review
The only thing that I'm missing is what I need to do in order to create the search service so that I can do a quick check to make sure it's on the main thread post creation.  Other than that all tests in that folder are passing on my end.
Attachment #8541802 - Flags: review?(gavin.sharp)
Comment on attachment 8541802 [details] [diff] [review]
bug-903456.patch

Awesome, thanks! Apologies for the delayed response, I've been taking a bit of time offline for the holidays.

This looks good overall. xpcom/tests/unit/test_bug930456_child.js needs an extra newline at the end, and you should add a descriptive commit message, per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment.

I've pushed this patch to try to make sure the test passes on all platforms:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a4edca3a829d

With those changes made and once the tryserver results are in, just go ahead and attach the updated patch here and we can mark it checkin-needed.
Attachment #8541802 - Flags: review?(gavin.sharp) → review+
Assignee: nobody → chrishood
(In reply to Chris from comment #12)
> The only thing that I'm missing is what I need to do in order to create the
> search service so that I can do a quick check to make sure it's on the main
> thread post creation.  Other than that all tests in that folder are passing
> on my end.

Missed this comment - the easiest way to trigger the creation of the search service is to access Services.search (in a context where the Services.jsm module has been loaded). The test coverage in your patch is great, though, so I think we'll get sufficient coverage from the try run (including existing tests).

I canceled the previous try push in favor of a more full-featured test set, so here's the new link to watch:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0f4f9291e778
It looks like there were some failures coming from the try server when I checked the results.  All 3 of them are marked as intermittent failures that don't appear to be related to this patch.
I have a checkin patch ready but I'd like to get your input on the try results before I post it since it looks like there were some issues, most of them look like unrelated intermittent failures but I'm concerned about the test run that did not complete successfully.
That run looks good to me. I re-triggered the two failed jobs just in case, but they're likely unrelated intermittent issues.
Attached patch bug-903456.patch (obsolete) — Splinter Review
This should be ready for checkin
Attachment #8541802 - Attachment is obsolete: true
Attachment #8541966 - Flags: review?(gavin.sharp)
Attachment #8541966 - Flags: review?(gavin.sharp) → review+
Thanks Chris! I've pushed that patch to fx-team, which should get merged to mozilla-central (and thus Nightly) within a day or two:

https://hg.mozilla.org/integration/fx-team/rev/169e19d13daa

For future reference, once you've received r+ you should include it in the commit message of the patch you attach (I added the r=gavin here). And you don't need to re-request review once r+ has been granted, unless you've changed things substantially or really do want an extra double-check. Feel free to needinfo people to ensure it gets checked in etc., though.
I'll keep that in mind, thanks Gavin.
Backed out for Android S4 bustage in https://hg.mozilla.org/integration/fx-team/rev/a91703c279ec

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1563444&repo=fx-team

Note: The bug number in the commit message was incorrect and should be fixed when this eventually relands.
Flags: needinfo?(chrishood)
Flags: needinfo?(gavin.sharp)
OK, so apparently child processes aren't that great on Android at the moment. So let's just disable the test by adding a "skip-if = os == "android"", similar to http://hg.mozilla.org/mozilla-central/annotate/a4a5d4fb5e2e/xpcom/tests/unit/xpcshell.ini#l54.

I was going to do that and re-push, but then I realized I also missed another thing: the tests should live in toolkit/components/search/tests/xpcshell/, rather than in xpcom/tests/unit/.

Chris, can you make those changes (and the change to the commit message) and re-attach a new patch?
Flags: needinfo?(gavin.sharp)
Attached patch bug-930456.patch (obsolete) — Splinter Review
Re-Uploading with a fixed commit message.  I marked the tests to skip on android based on comments found in bug 966467.  Marking this patch for re-review to make sure that this is okay.
Attachment #8541966 - Attachment is obsolete: true
Flags: needinfo?(chrishood)
Attachment #8542257 - Flags: review?(gavin.sharp)
Sorry, I made my comment before your comment showed up.  I'll make the necassary fixes and re-upload another patch for checkin.
Attachment #8542257 - Flags: review?(gavin.sharp)
One of the setup lines in toolkit/components/search/tests/xpcshell/xpcshell.ini says skip-if = toolkit == 'android'

Does that automatically make these tests in this xpcshell.ini skip if they are run on android?
Ah, yes it does! So no additional change needed, then.

(It's a bit odd that we disable all of the search xpcshell tests on Android, I wonder why we do that...)
okay, the only thing that I need to do then is to figure out how to get the test to run properly from the search xpcshell test directory (They fail pretty spectacularly if I just try to drag/drop/run them).  I'll upload another patch for review/checkin once I've got that sorted out.
How are they failing exactly? Could you maybe attach an updated patch and I could take a look to see if I see anything wrong?
Attached patch bug-930456_busted.patch (obsolete) — Splinter Review
I'm getting a file not found error, it looks like it doesn't know how to find the services.jsm file from the search directory.  It's likely that theres some additional setup that will be needed for this test to pass when run as a part of the search xpcshell tests unless it's modified.
Attachment #8542257 - Attachment is obsolete: true
Oh, I see the issue:

resource:///modules/Services.jsm

in the test should instead be:

resource://gre/modules/Services.jsm

The search head.js file (toolkit/components/search/tests/xpcshell/head_search.js) registers a XULAppInfo which probably makes the two not equivalent, where they usually would be.
I looked in head_search.js and it looks like its actually already being loaded at line 10.

When I run the test in a child thread I'm getting a failure before the test ever actually runs because head_search.js attempts to load the search service.  Since we have plenty of coverage in the other tests of the search service in a main thread is it possible to get away with having a negative test which runs in a child thread that is expected to fail with (NS_ERROR_NOT_AVAILABLE)?  Do you know how I would go about setting this up?
Oh, good point, no need to re-import Services.jsm.

I think I understand the issue, after reading your comment and doing a bit of debugging. head_search.js registers an alternative XULAppInfo implementation, which also overrides nsIXULRuntime. It doesn't have a proper implementation of the nsIXULRuntime.processType getter, which means your test doesn't end up checking the right thing. There's a secondary problem, which is that set*Pref calls throw in child processes, and so head_search.js needs to avoid calling them when run in a child process.

Let me attach a patch that you should be able to combine with yours to make this all work!
Attached patch additional fix (obsolete) — Splinter Review
This patch does two things:
- make the nsIXULRuntime implementation that head_search.js sets up to override the default one properly forward the state of .processType, by just delegating to the actual core implementation of nsIXULRuntime
- avoids attempting to create a profile (which just fails silently) or setting prefs (which throws an exception) in child processes

Chris, can you combine this with your patch and see if that solves all the issues you were seeing?
Attachment #8542605 - Attachment is obsolete: true
Attachment #8542714 - Flags: feedback?(chrishood)
Comment on attachment 8542714 [details] [diff] [review]
additional fix

This did the trick, I applied it on top of my patch and all relevant tests are passing.  I'll post a patch for checkin shortly.
Attachment #8542714 - Flags: feedback?(chrishood) → feedback+
Attached patch bug_930456.patch (obsolete) — Splinter Review
Removed the isChild declaration since it's now being declared in search_head and ran all tests to make sure they still passed.  Thanks a bunch for your help on this Gavin.

I'm marking this for checkin but let me know if you'd like to do another review first.
Attachment #8542742 - Flags: checkin+
Comment on attachment 8542742 [details] [diff] [review]
bug_930456.patch

checkin+ is for things that have been checked in already.
Attachment #8542742 - Flags: checkin+ → checkin?
Sorry, I'll keep that in mind.
Comment on attachment 8542742 [details] [diff] [review]
bug_930456.patch

There's an issue with this patch that I need to fix, I'll update another one later today.
Attachment #8542742 - Flags: checkin?
Attached patch bug-930456.patch (obsolete) — Splinter Review
Fixed and merged the two patches together.  This should be good to go for a final review prior to checkin (Just to be extra safe).
Attachment #8542742 - Attachment is obsolete: true
Attachment #8543098 - Flags: review?(gavin.sharp)
Comment on attachment 8543098 [details] [diff] [review]
bug-930456.patch

>diff --git a/toolkit/components/search/tests/xpcshell/head_search.js b/toolkit/components/search/tests/xpcshell/head_search.js

>+//Don't set service prefs for tests run in a child thread since the search service is not available.
>+if(!isChild) {

I would just rephrase this comment as "Can't set prefs in the child process" - the relevant limitation isn't really related to the search service itself (but rather the pref service enforcing this behavior).

And a formatting nit: add spaces after "if" and "//".

I made those changes and pushed this to try one last time:

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

Assuming that goes green, this is good to go.

Thanks for your persistence on this one, Chris.
Attachment #8543098 - Flags: review?(gavin.sharp) → review+
Attached patch combined patchSplinter Review
Attachment #8542714 - Attachment is obsolete: true
Attachment #8543098 - Attachment is obsolete: true
Pushed this to fx-team again:

https://hg.mozilla.org/integration/fx-team/rev/5e34d8220c13
No longer blocks: 1045928
Target Milestone: --- → Firefox 37
https://hg.mozilla.org/mozilla-central/rev/5e34d8220c13
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Iteration: --- → 37.3
Flags: qe-verify?
Seems to be covered by automated tests. Set as qe-verify+ if you think any manual testing is needed.
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.