Closed Bug 1086996 Opened 6 years ago Closed 6 years ago

Regression: touch video controls are busted

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox33 --- unaffected
firefox34 --- unaffected
firefox35 --- unaffected
firefox36 + fixed
fennec 36+ ---

People

(Reporter: snorp, Assigned: bholley)

References

Details

(Keywords: regression, reproducible)

Attachments

(1 file)

I can't bring up the video controls at all while a video is playing. Recent regression I think.
Looks like it first started in the 10-21-2014 Nightly.
Ok that regression range seems....unlikely
Ok, this range looks more reasonable:

Last good revision: acce1a807efb
First bad revision: 2681f9b134c2
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=acce1a807efb&tochange=2681f9b134c2
I see this message in logcat when touching the video:

W/GeckoConsole(17065): [JavaScript Error: "Error: Permission denied to pass object to privileged code"]

I guess something here busted fennec's chrome. Wes/Bobby know what's going on?
Flags: needinfo?(wjohnston)
Flags: needinfo?(bobbyholley)
tracking-fennec: --- → ?
There are two places in videocontrols.xml where we dispatch an event to chrome, like this:

  this.dispatchEvent(new CustomEvent("VideoBindingAttached"));

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1711
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.xml#1779

I assume the <constructor> line is failing, breaking the binding.

When I added this code, I modeled it after the "PluginBindingAttached" event here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/pluginProblem.xml#78
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
Blocks: 1085787
Summary: Video controls busted → Touch video controls busted
Comment on attachment 8509401 [details] [diff] [review]
Stop wrapping XBL handlers into the content compartment. v1

The basic problem with this patch is that nsXBLPrototypeHandler.cpp creates the bound handlers in the XBL scope, but then wraps them into the content scope before invoking them (there doesn't seem to be any reason for this anymore, AFAICT, so my patch just removes it).

When the handler is executed from the XBL scope, we run into problems trying to pass |this|, because |this| is anonymous content that lives in the XBL scope, and thus isn't something that content should be allowed to touch.
Attachment #8509401 - Flags: review?(bugs)
(In reply to Bobby Holley (:bholley) from comment #7)
> Created attachment 8509401 [details] [diff] [review]
> Stop wrapping XBL handlers into the content compartment. v1

This fixes the issue, thanks! Tested on b2g desktop.
Attachment #8509401 - Flags: feedback?(martijn.martijn) → feedback+
I think you could use something like SpecialPowers.registerConsoleListener to write a mochitest for videocontrols:
http://mxr.mozilla.org/mozilla-central/source/layout/mathml/tests/test_bug553917.html?force=1#116
Mochitests are run on Android and b2g emulator, so that way it would have been caught earlier.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #10)
> I think you could use something like SpecialPowers.registerConsoleListener
> to write a mochitest for videocontrols:

That would only be to catch this specific issue, yes? There are various ways to do that, but the bigger problem is that we don't appear to have any coverage on CI that checks whether the touch controls work at all.
Comment on attachment 8509401 [details] [diff] [review]
Stop wrapping XBL handlers into the content compartment. v1

smaug wants bz to review this instead.
Attachment #8509401 - Flags: review?(bugs) → review?(bzbarsky)
Blocks: 1081990
(In reply to Bobby Holley (:bholley) from comment #12)
> smaug wants bz to review this instead.
Because the code to be removed was added in Bug 821850.
Comment on attachment 8509401 [details] [diff] [review]
Stop wrapping XBL handlers into the content compartment. v1

r=me
Attachment #8509401 - Flags: review?(bzbarsky) → review+
Pushed with a test for the specific XBL issue (though nothing for the broader problem of not having touchControls coverage):

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/00f96ab44761
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1448d42a32c6
We do some testing of video controls in a gaia-ui-test, that's why duped bug 1086265 was filed.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #16)
> We do some testing of video controls in a gaia-ui-test, that's why duped bug
> 1086265 was filed.

Any way we can get those tests showing up in treeherder?
Status: NEW → ASSIGNED
Keywords: reproducible
OS: Mac OS X → Android
Hardware: x86 → ARM
Summary: Touch video controls busted → Regression: touch video controls are busted
https://hg.mozilla.org/mozilla-central/rev/00f96ab44761
https://hg.mozilla.org/mozilla-central/rev/1448d42a32c6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Duplicate of this bug: 1085787
tracking-fennec: ? → 36+
Flags: needinfo?(wjohnston)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.