Closed Bug 1125477 Opened 5 years ago Closed 4 years ago

Enable the DOM TV tests on platforms where they currently fail

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: RyanVM, Assigned: mantaroh)

References

Details

Attachments

(4 files, 5 obsolete files)

13.18 KB, text/plain
Details
69.40 KB, patch
mantaroh
: review+
Details | Diff | Splinter Review
10.80 KB, patch
selin
: review+
smaug
: review+
Details | Diff | Splinter Review
724 bytes, patch
mantaroh
: review+
Details | Diff | Splinter Review
Attached file 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.
Assignee: nobody → jacheng
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.
Flags: needinfo?(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?
Flags: needinfo?(ryanvm) → needinfo?(selin)
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
Flags: needinfo?(selin)
Hi Ehsan,

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

Really appreciate it. :)
Flags: needinfo?(ehsan)
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,
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(amarchesini)
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(amarchesini)
Blocks: 1200133
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.
Attachment #8679300 - Flags: review?(selin)
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|.
Attachment #8679300 - Flags: review?(selin) → review+
Assignee: jacheng → mantaroh
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.
Attachment #8679300 - Attachment is obsolete: true
Attachment #8681089 - Flags: review?(selin)
This patch is remove Navigator::HasTvSupport check.The preference of |dom.testing.tv_enabled_for_hosted_apps| will remove bug 1200133.
Attachment #8679304 - Attachment is obsolete: true
Attachment #8681092 - Flags: review?(selin)
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);
  });
}
Attachment #8681089 - Flags: review?(selin) → review+
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.
Attachment #8681092 - Flags: review?(selin) → review+
Thanks Sean,

(In reply to Sean Lin [:seanlin] from comment #12)
I modified the comment #12.
This patch is carrying forward r+ from seanlin.
Attachment #8681089 - Attachment is obsolete: true
Attachment #8681773 - Flags: review+
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?
Attachment #8681092 - Attachment is obsolete: true
Attachment #8681782 - Flags: review?(selin)
Attachment #8681782 - Flags: review?(bugs)
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.
Attachment #8681782 - Flags: review?(selin) → review+
Attachment #8681782 - Flags: review?(bugs) → review+
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?
Attachment #8683465 - Flags: feedback?(selin)
Attachment #8683465 - Attachment description: Part3. Remove the test code of removing permission before test. → Part3. Skip debug mochitest at dom/tv.
Attachment #8683465 - Flags: feedback?(selin) → review?(selin)
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.
Attachment #8683465 - Flags: review?(selin) → review+
Thanks Sean,

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c718a3ae242b
Attachment #8683465 - Attachment is obsolete: true
Attachment #8685826 - Flags: review+
Keywords: checkin-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.