1125477 - Enable the DOM TV tests on platforms where they currently fail

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Attachment: B2G/e10s failure log

Shortly after they landed, the DOM TV tests were disabled on most platforms due to hitting bug 1091322. However, a recent Try run confirms that they can be re-enabled on all platforms except for B2G and desktop platforms w/ e10s enabled.

The failure mode for B2G and desktop e10s is the same as far as I can tell. I've included it as an attachment here for posterity.

Comment 1

[James Cheng[:JamesCheng]]

Hi Ryan,
After analysis, 
The root cause is that content process cannot embed-apps. Embed-apps is restricted to in-proc apps.

https://dxr.mozilla.org/mozilla-central/source/dom/html/nsGenericHTMLFrameElement.cpp#446-449

I can successfully run the test case directly instead of running in hosted app.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

I'm not the right person to be asking. I just filed the bug for tracking this work :) Maybe Sean can help since he's the one who wrote the tests?

Comment 3

[Sean Lin [:seanlin] (inactive from Mar 2016)]

As what's mentioned in comment 1, bug 1059662 began to disallow embedded in-proc apps for OOP. So under e10s tests, "tv" permission, which is supposed to be pushed to the embedded app as it is in non-OOP scenarios, fails to apply to the app window.

If TV tests needs to be run on e10s, here is one possible fix. Instead of running test logic in a hosted app in mochitests, simply apply necessary preferences (|dom.tv.enabled|, |dom.testing.tv_enabled_for_hosted_apps|) and permissions ("tv") to the test page and run the tests without installing any app.

Furthermore, now that an embedded app is not required in the proposal above, preference |dom.testing.tv_enabled_for_hosted_apps| doesn't appear necessary any more. We may remove it as well as function "Navigator::HasTVSupport" [1]; then replace |Func="Navigator::HasTVSupport"| with |AvailableIn=CertifiedApps| for all relevant WebIDLs. While running tests, |dom.ignore_webidl_scope_checks| could be used to bypass that restriction.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#2411-2440

Comment 4

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Hi Ehsan,

Could you share your feedback or advises for the proposal in comment 3?

Really appreciate it. :)

Comment 5

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Hi Andrea or Ben,

We realize Ehsan is away. As per Andrew's recommendation, could either of you have a quick look at comment 3 and share your opinion?

Thanks,

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I haven't really been keeping up with the nested process architecture, but as long as the tests actually test the use case you expect for production then I am fine with whatever changes we need to make to the tests themselves.

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part1. Modify dom/tv mochitest in order to enable e10s/gonk.

Hi Sean,

I working on bug 1200133. and I would like to fix this test issue.
I modified as the simple page mochitest instead of running test logic in a hosted app, and enable e10s tests and gonk test.

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: WIP: Part2. Use permission check using AvailableIn=CertifiedApps in dom/tv interfaces.

Comment 9

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8679300 [details] [diff] [review]
Part1. Modify dom/tv mochitest in order to enable e10s/gonk.

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

Thanks for taking this. r=me once the following comments are addressed.

::: dom/tv/test/mochitest/head.js
@@ +2,5 @@
>  
> +function setupPrefsAndPermissions(callback) {
> +  setupPrefs(function() {
> +    SpecialPowers.addPermission("tv", true, document);
> +    this.setTimeout(callback, 0);

|callback()| appears well enough. Or you may rewrite as follows

SpecialPowers.pushPermissions([
  { "type": "tv", "allow": 1, "context": document }
], callback);

Same for other occurrences in this file.

@@ +8,5 @@
>  }
>  
> +function setupPrefs(callback) {
> +  SpecialPowers.pushPrefEnv({"set": [["dom.tv.enabled", true],
> +                                     ["dom.testing.tv_enabled_for_hosted_apps", true]]}, function() {

Just a reminder. Once |Navigator::HasTVSupport| is deprecated, we may replace this one with |dom.ignore_webidl_scope_checks|.

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part1. Modify dom/tv mochitest in order to enable e10s/gonk.

Thanks Sean,

Previous patch failed mochitest sometime. The reason of failure is that preference of |dom.testing.tv_enabled_for_hosted_apps| is null when fired EITBroadcasted event.
So I modified some test codes in order to pass the test always.

Comment 11

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part2. Use permission check using AvailableIn=CertifiedApps in dom/tv interfaces.

This patch is remove Navigator::HasTvSupport check.The preference of |dom.testing.tv_enabled_for_hosted_apps| will remove bug 1200133.

Comment 12

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8681089 [details] [diff] [review]
Part1. Modify dom/tv mochitest in order to enable e10s/gonk.

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

::: dom/tv/test/mochitest/head.js
@@ +3,5 @@
> +function setupPrefsAndPermissions(callback) {
> +  setupPrefs(function() {
> +    SpecialPowers.pushPermissions([
> +                           {"type":"tv", "allow":1, "context":document}
> +                           ], callback);

nit: fix indentation.

SpecialPowers.pushPermissions([
  {"type":"tv", "allow":1, "context":document}
], callback);

@@ +22,3 @@
>  }
>  
> +function popPrefs(callback) {

nit: now that only |removePrefsAndPermissions| calls |popPrefs|, you may simply merge the two functions.

function removePrefsAndPermissions(callback) {
  SpecialPowers.popPrefEnv(function() {
    SpecialPowers.popPermissions(callback);
  });
}

Comment 13

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8681092 [details] [diff] [review]
Part2. Use permission check using AvailableIn=CertifiedApps in dom/tv interfaces.

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

r=me once the comments are addressed. And since there are some WebIDL changes in this patch, it may also require DOM peer review.

::: dom/tv/test/mochitest/head.js
@@ +10,5 @@
>  
>  function setupPrefs(callback) {
>    SpecialPowers.pushPrefEnv({"set": [["dom.tv.enabled", true],
> +                                     ["dom.testing.tv_enabled_for_hosted_apps", true],
> +                                     ["dom.ignore_webidl_scope_checks", true]]}, function() {

nit: |dom.testing.tv_enabled_for_hosted_apps| is no longer needed.

Comment 14

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part1. Modify dom/tv mochitest in order to enable e10s/gonk. (carry r+ from seanlin)

Thanks Sean,

(In reply to Sean Lin [:seanlin] from comment #12)
I modified the comment #12.
This patch is carrying forward r+ from seanlin.

Comment 15

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part2. Use permission check using AvailableIn=CertifiedApps in dom/tv interfaces.

Thanks Sean,

(In reply to Sean Lin [:seanlin] from comment #13)
> >  function setupPrefs(callback) {
> >    SpecialPowers.pushPrefEnv({"set": [["dom.tv.enabled", true],
> > +                                     ["dom.testing.tv_enabled_for_hosted_apps", true],
> > +                                     ["dom.ignore_webidl_scope_checks", true]]}, function() {
> 
> nit: |dom.testing.tv_enabled_for_hosted_apps| is no longer needed.
|dom.testing.tv_enabled_for_hosted_apps| is using from |TVServiceFactory::autoCreateService|[1].
We can remove this preference. However we have better to use |dom.ignore_webidl_scope_checks| instead that preference.

[1] https://dxr.mozilla.org/mozilla-central/rev/96377bdbcdf3e444a22aeaa677da696243b00d98/dom/tv/TVServiceFactory.cpp#31

Hi Olli,
We changed checking mechanism to using |AvaiableApp=Certified| about TV Manager APIs.
So I modified the WebIDL define.

Could you review this change?

Comment 16

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8681782 [details] [diff] [review]
Part2. Use permission check using AvailableIn=CertifiedApps in dom/tv interfaces.

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

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #15)
> (In reply to Sean Lin [:seanlin] from comment #13)
> > >  function setupPrefs(callback) {
> > >    SpecialPowers.pushPrefEnv({"set": [["dom.tv.enabled", true],
> > > +                                     ["dom.testing.tv_enabled_for_hosted_apps", true],
> > > +                                     ["dom.ignore_webidl_scope_checks", true]]}, function() {
> > 
> > nit: |dom.testing.tv_enabled_for_hosted_apps| is no longer needed.
> |dom.testing.tv_enabled_for_hosted_apps| is using from
> |TVServiceFactory::autoCreateService|[1].
> We can remove this preference. However we have better to use
> |dom.ignore_webidl_scope_checks| instead that preference.
> 
Looks good to me.

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part3. Skip debug mochitest at dom/tv.

Thanks Sean, 

I tried this patch on try server. It was happen test failure some environments.(OSX debug).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d10ca134a2dd

This reason is timing issue. When firing the EITBroadcasted event, test code removed the |dom.testing.tv_enabled_for_hosted_apps| prefs. So EITBroadcasted callback can't get the correct TVService.

This issue will fix when land bug 1200133. So I'd like to disable this mochitest when running the debug build test.
What do you think about this plan?

Comment 18

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8683465 [details] [diff] [review]
Part3. Skip debug mochitest at dom/tv.

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

::: dom/tv/test/mochitest/mochitest.ini
@@ +1,3 @@
>  [DEFAULT]
>  support-files = head.js
> +skip-if = (debug || asan)

It might be better to just disable OSX debug based on the test result in comment 17. How about the following?

skip-if = (os == 'mac' && debug) || asan

(And you may apply 'skip-if' to the affected test cases only if not all of them are affected by this issue.)

nit: please add a comment like "# Bug 1200133", so we'll remove this after fixing that bug.

Comment 19

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Part3. Remove the test code of removing permission before test.

Thanks Sean,

I modified some code about comment #18.
Carrying forward r+ from Sean.

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c47167e26a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f1cf9ec0b37
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a2246d306f

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/6c47167e26a8
https://hg.mozilla.org/mozilla-central/rev/6f1cf9ec0b37
https://hg.mozilla.org/mozilla-central/rev/f4a2246d306f