Closed Bug 1395827 Opened 2 years ago Closed 2 years ago

Eliminate NO_REMOTE_TYPE="" and make xpcshell spawn DEFAULT_REMOTE_TYPE="web" child processes (also the unused {nsIXULRuntime,nsXULAppInfo}::EnsureContentProcess())

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: asuth, Assigned: asuth)

References

Details

Attachments

(1 file)

Currently, xpcshell e10s tests create a ContentParent of type NO_REMOTE_TYPE which is "".  This caused test failures in xpcshell for the push notification tests in bug 1383905 where I tried to fix ServiceWorker e10s snafus to only dispatch notifications to DEFAULT_REMOTE_TYPE="web" content processes.

Very quick code archaeology suggests that there was no actual intent behind the choice of NO_REMOTE_TYPE.  I can see a rationale of "the xpcshell e10s child process is unspecialized and so it would be misleading to give it a type", but I think the counter argument is "it's counterproductive to have a remote type that we'll never see in a running Firefox browser and that therefore requires specialization in app logic".  Along those lines, if we really want the former rationale, I think an empty string is a dangerous choice, as is the choice of getting it via default argument of GetNewOrUsedBrowserProcess().  Far better to have "xpcshell-content-child-if-you-are-trying-to-compare-against-this-you-need-to-be-a-browser-mochitest-test".

We could also abstract away the issue somewhat by putting methods on ContentParent that take the desired remote type and internally compensates for xpcshell-ness.  Like CheckRemoteType(DEFAULT_REMOTE_TYPE, one of: eMustBeThisOrBeXpcshell, eMustNotBeThisOrBeXpcshell).

Searchfox links for the xpcshell case:
- callsite of GetNewOrUsedBrowserProcess():
https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/toolkit/xre/nsEmbedFunctions.cpp#904
- prototype:
https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/ipc/ContentParent.h#172
- constant declaration:
https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/ipc/ContentParent.h#39
Attachment #8903461 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/7af2b462a50707de5bf79a3f118fee5303c7ab72
Bug 1395827 - Eliminate NO_REMOTE_TYPE and default GetNewOrUsedBrowserProcess() remote type arg. r=billm
https://hg.mozilla.org/mozilla-central/rev/7af2b462a507
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8903461 [details] [diff] [review]
Eliminate NO_REMOTE_TYPE and default GetNewOrUsedBrowserProcess() remote type arg

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1383905 needs this so it can be uplifted.

[User impact if declined]:
Bug 1383905 that is an important bug will have its xpcshell tests fail and get backed out.

[Is this code covered by automated tests?]:
It's complicated.  This bug changes the "remote type" content process type used in xpcshell tests from "" to "web", our default.  Very little logic cares about remote types in Firefox, especially under xpcshell tests.  And if they did care, they would be happy about this change because the value without this change is a synthetic value that we never use in real Firefox.  I very thoroughly investigated all oranges from my try push in comment 1 to make sure there were no surprise failures.

1: E10SUtils.jsm cares a little, as does some sandboxing-related logic in Content{Parent,Child} that keys off of the "file" remote type which is an orthogonal issue.

[Has the fix been verified in Nightly?]:
Yes, as part of bug 1383905.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
This is helping out bug 1383905, land this patch with that one.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
This only affects xpcshell tests, which are all still green and have their own binary.

[String changes made/needed]:
None.
Attachment #8903461 - Flags: approval-mozilla-beta?
Comment on attachment 8903461 [details] [diff] [review]
Eliminate NO_REMOTE_TYPE and default GetNewOrUsedBrowserProcess() remote type arg

Required by bug 1383905. Beta56+.
Attachment #8903461 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.