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

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: manifest-filtering

Gavin asked for something like this in https://bugzilla.mozilla.org/show_bug.cgi?id=966467#c6.

Comment 3

[Benjamin Smedberg]

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.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: manifest-filtering v2

I think I can finally use this for something.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Looks like that patch belongs on bug 930243?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8496257 [details] [diff] [review]
manifest-filtering v2

OK, I'll move it.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

This should be pretty straightforward now that we have bug 930243.

Comment 8

[Chris]

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?

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

ni to answer comment 8

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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

Comment 11

[Chris]

Thanks, I'll update with a patch when I'm ready for a review.

Comment 12

[Chris]

Attachment: bug-903456.patch

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.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(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

Comment 15

[Chris]

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.

Comment 16

[Chris]

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.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

That run looks good to me. I re-triggered the two failed jobs just in case, but they're likely unrelated intermittent issues.

Comment 18

[Chris]

Attachment: bug-903456.patch

This should be ready for checkin

Comment 19

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 20

[Chris]

I'll keep that in mind, thanks Gavin.

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 23

[Chris]

Attachment: bug-930456.patch

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.

Comment 24

[Chris]

Sorry, I made my comment before your comment showed up.  I'll make the necassary fixes and re-upload another patch for checkin.

Comment 25

[Chris]

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?

Comment 26

[:Gavin Sharp [email: [e-mail]]]

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

Comment 27

[Chris]

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.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 29

[Chris]

Attachment: bug-930456_busted.patch

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.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 31

[Chris]

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?

Comment 32

[:Gavin Sharp [email: [e-mail]]]

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!

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Attachment: additional fix

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?

Comment 34

[Chris]

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.

Comment 35

[Chris]

Attachment: bug_930456.patch

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.

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8542742 [details] [diff] [review]
bug_930456.patch

checkin+ is for things that have been checked in already.

Comment 37

[Chris]

Sorry, I'll keep that in mind.

Comment 38

[Chris]

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.

Comment 39

[Chris]

Attachment: bug-930456.patch

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

Comment 40

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 41

[:Gavin Sharp [email: [e-mail]]]

Attachment: combined patch

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Pushed this to fx-team again:

https://hg.mozilla.org/integration/fx-team/rev/5e34d8220c13

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5e34d8220c13

Comment 44

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Seems to be covered by automated tests. Set as qe-verify+ if you think any manual testing is needed.