1243378 - Enable the browser_devices_get_user_media_about_urls.js test for e10s

Status: RESOLVED FIXED

(Firefox :: Site Permissions, defect)

Description

[Florian Quèze [:florian]]

I (mostly) fixed this test for e10s compatibility in bug 1239985, but to get the test to pass with e10s, I had to replace the code waiting for load events with timers (attachment 8709996 [details] [diff] [review]). This is not a hack we want to land.

I tried various things to attempt to get load events for this test but nothing I tried worked.
Some things I tried:
- BrowserTestUtils.browserLoaded
- "load" events from a ContentTask
- webProgress.addProgressListener from the parent process
- webProgress.addProgressListener from a ContentTask

All of these things felts like hacks anyway, I think it would really be preferable if someone could figure out why these about: pages don't fire load events like the http(s) pages do.

Comment 1

[Mark Banner (:standard8)]

Note: I've just updated the spreadsheet for the tests saying we should fix this - Hello is working towards enabling in e10s mode in bug 1154273.

Its possible that a test here would have caught the e10s side of bug 1249365 when it landed (although I'm not 100% sure about that, but at least having this covered would give me more confidence).

Comment 2

[Florian Quèze [:florian]]

When running the test with e10s, there are some error messages in the terminal before the test gets stuck:

JavaScript error: resource://gre/modules/RemoteAddonsChild.jsm, line 258: TypeError: can't convert undefined to object
4 INFO Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIChannel.open2]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/RemoteAddonsParent.jsm :: AboutProtocolParent.openChannel :: line 276"  data: no]"]
AboutProtocolParent.openChannel@resource://gre/modules/RemoteAddonsParent.jsm:276:20
AboutProtocolParent.receiveMessage@resource://gre/modules/RemoteAddonsParent.jsm:233:16

JavaScript error: chrome://global/content/browser-child.js, line 316: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: Component returned failure code: 0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS) [nsIWebNavigation.loadURIWithOptions]
5 INFO Console message: [JavaScript Error: "TypeError: can't convert undefined to object" {file: "resource://gre/modules/RemoteAddonsChild.jsm" line: 258}]
6 INFO Console message: [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: Component returned failure code: 0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS) [nsIWebNavigation.loadURIWithOptions]" {file: "chrome://global/content/browser-child.js" line: 316}]

I'm not sure if this is causing the load notifications to never be sent, or if it's just additional noise as a side effect of the bug. I suspect the latter.

Comment 3

[Florian Quèze [:florian]]

Most problems here seem to be caused by the shims forwarding about: requests in the child process to the same about modules in the parent process. These shims are meant mostly for add-ons, and given that this test isn't meant to test the shims, it seems reasonable to bypass them here by registering the fake loop about module in the child process.

The JS errors I mentioned in comment 2 (and a fatal assertion when running a debug build: "need one securityflag from nsILoadInfo to perform security checks" from nsContentSecurityManager.cpp) disappear if I replace channel.open2() at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsParent.jsm?rev=44b39229cd33#276 with channel.open() (ie. removing the security checks on the load). However, that change isn't sufficient to have working load events on the page, so there must be a second shim bug that I haven't figured out.

Try is green https://treeherder.mozilla.org/#/jobs?repo=try&revision=136e6f8db78c&selectedJob=19091146 except for debug e10s on mac where the child process crashes with a fatal assertion ("Assertion failure: mBlockDOMContentLoaded, at dom/base/nsDocument.cpp:5225"). I think it's fine to disable the test just there so that we can enable it for e10s everywhere else.

Comment 4

[Florian Quèze [:florian]]

Attachment: Register the fake Loop modules in the child

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8739016 [details] [diff] [review]
Register the fake Loop modules in the child

Review of attachment 8739016 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, though I have a recommendation for an improvement. See below.

::: browser/base/content/test/general/browser_devices_get_user_media_about_urls.js
@@ +210,4 @@
>  
>    Task.spawn(function () {
> +    yield new Promise(resolve => {
> +      mm.addMessageListener("Test:RegisteredFakeLoopModules", function listener() {

I think we can get rid of this Register / Registered messaging boilerplate if we yield a ContentTask instead.

I believe you can do the same thing for unregistering as well, right? Or is it necessary that you keep the factory in scope so that you can remove it? (I seem to recall that you can get a reference to the currently registered factory via the registrar...)

I think that'd be cleaner than using the call / response pattern here.

Comment 6

[Florian Quèze [:florian]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)

> I think we can get rid of this Register / Registered messaging boilerplate
> if we yield a ContentTask instead.
> 
> I believe you can do the same thing for unregistering as well, right? Or is
> it necessary that you keep the factory in scope so that you can remove it?
> (I seem to recall that you can get a reference to the currently registered
> factory via the registrar...)
> 
> I think that'd be cleaner than using the call / response pattern here.

I first tried with ContentTask, but (I thought) I had to keep the factory and the generated class IDs in scope. I'm not sure bouncing the class IDs back and forth between the processes is cleaner (I assume we would have to stringify them, and re-create the object based on the string to unregister). Or is there a way to get both the class ID and the factory back without having to save them?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #5)
> 
> > I think we can get rid of this Register / Registered messaging boilerplate
> > if we yield a ContentTask instead.
> > 
> > I believe you can do the same thing for unregistering as well, right? Or is
> > it necessary that you keep the factory in scope so that you can remove it?
> > (I seem to recall that you can get a reference to the currently registered
> > factory via the registrar...)
> > 
> > I think that'd be cleaner than using the call / response pattern here.
> 
> I first tried with ContentTask, but (I thought) I had to keep the factory
> and the generated class IDs in scope. I'm not sure bouncing the class IDs
> back and forth between the processes is cleaner (I assume we would have to
> stringify them, and re-create the object based on the string to unregister).
> Or is there a way to get both the class ID and the factory back without
> having to save them?

I think if you have the contract ID (which you can pass down through the ContentTask), then you have enough information to get the class ID and the factory. Example: http://searchfox.org/mozilla-central/source/testing/specialpowers/content/MockPermissionPrompt.jsm#38

I should note that this is good fodder for follow-up work. The test passes as is, and if you have more tests to go through, that's probably higher priority than attending to my nit here, and a follow-up bug could be filed.

Comment 8

[Florian Quèze [:florian]]

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

Comment 9

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)

> Try is green
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=136e6f8db78c&selectedJob=19091146 except for debug
> e10s on mac where the child process crashes with a fatal assertion
> ("Assertion failure: mBlockDOMContentLoaded, at
> dom/base/nsDocument.cpp:5225"). I think it's fine to disable the test just
> there so that we can enable it for e10s everywhere else.

This "Assertion failure: mBlockDOMContentLoaded" failure is actually intermittent, and failed on Linux debug too on my second try push, so I'll disable the test for e10s && debug, not just on Mac.

I'll land with the code changed to using ContentTask, Mike looked at the new code in my second try push and confirmed I can carry forward this r+.

Comment 10

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/f20a435525b777b6963ce20333311216e0523bf7
Bug 1243378 - Enable the browser_devices_get_user_media_about_urls.js test for e10s, r=mconley.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/f20a435525b7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> This "Assertion failure: mBlockDOMContentLoaded" failure is actually
> intermittent, and failed on Linux debug too on my second try push, so I'll
> disable the test for e10s && debug, not just on Mac.

Is there a bug filed tracking the issue on debug? In general, it would be good to annotate the manifest with a pointer to that bug so we don't have to go digging through blame to find it.

Comment 13

[Florian Quèze [:florian]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #12)
> (In reply to Florian Quèze [:florian] [:flo] from comment #9)
> > This "Assertion failure: mBlockDOMContentLoaded" failure is actually
> > intermittent, and failed on Linux debug too on my second try push, so I'll
> > disable the test for e10s && debug, not just on Mac.
> 
> Is there a bug filed tracking the issue on debug?

There wasn't, but the test no longer exists (removed by bug 1287827).