845555 - After showing statistics, context menu item to switch to hide statistics instead of offering to show statistics again

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Go to https://marketplace.firefox.com/developers/partners - there is a HTML5 video halfway down the page.

Play it. Right click on it while it is playing, then select "show statistics". Statistics show properly.

Now right click on it again, and there is no option to "hide statistics". Clicking on "show statistics" again does not seem to have any effect, as the statistics still continue to show.

Comment 2

[Matthew Gregan [:kinetik]]

Pretty sure this used to work.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Last good nightly: 2013-02-14
First bad nightly: 2013-02-15

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aceeea086ccb&tochange=953b1db7a246

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Likely cause:
Bobby Holley — Bug 834697 - Enable XBL scopes, and disable assertion. r=bz,me

Comment 5

[Bobby Holley (:bholley)]

jaws, if you can point me to the line of video code that appears to be malfunctioning, I can almost certain explain why. :-)

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bobby Holley (:bholley) from comment #5)
> jaws, if you can point me to the line of video code that appears to be
> malfunctioning, I can almost certain explain why. :-)

I believe this line is the suspect one, and it is probably happening now because this would cross compartments:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#398

Comment 7

[Bobby Holley (:bholley)]

Ok, so I'm pretty sure the issue here is with the mozMediaStatisticsShowing expando.

The chrome code in nsContextMenu.js accesses it as target.wrappedJSObject.mozMediaStatisticsShowing, which means that it will appear as an expando in the content scope.

The XBL code accesses it as video.mozMediaStatisticsShowing, which previously worked, because the XBL was running in the same scope as content. Now the XBL runs with Xrays, so it doesn't appear.

Ideally, this wouldn't be visible to content at all, because both chrome and the XBL are accessing the object via Xray wrappers, so we could just put it as an Xray expando. However, Xray expandos are per-origin, and XBL doesn't actually run with SystemPrincipal, but rather with nsExpandedPrincipal(contentPrincipal), which is somewhere between the two.

Long story short - easiest fix here is just to replace video.mozMediaStatisticsShowing with XPCNativeWrapper.unwrap(video).mozMediaStatisticsShowing in videocontrols.xbl.

It's important to use XPCNativeWrapper.unwrap(video) rather than video.wrappedJSObject, because the former will work if we flip the pref and turn off XBL scopes (and the associated Xray vision), but the latter will break.

Comment 9

[:Gijs (he/him)]

Attachment: Patch

This fixes the menu being completely broken (trunk, aurora) and halfway broken (beta).

Comment 10

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #9)
> Created attachment 737833 [details] [diff] [review]
> Patch

https://tbpl.mozilla.org/?tree=Try&rev=846afe30bcbe

Comment 11

[Bobby Holley (:bholley)]

Err, cross-posting from bug 862213, which appears to be closed now.

So, why does this expando need to live in content? Does content set it somewhere? It seems like it should just live on the Xray wrapper (accessible to the XBL code but not to content)...

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 737833 [details] [diff] [review]
Patch

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

::: toolkit/content/widgets/videocontrols.xml
@@ +463,5 @@
>                      this.video.addEventListener("media-showStatistics", this._handleCustomEventsBound, false, true);
>                  },
>  
>                  handleCustomEvents : function (e) {
> +                    this.toggleStatistics();

How come we don't need to check if the event is trusted anymore?

Comment 13

[:Gijs (he/him)]

Comment on attachment 737833 [details] [diff] [review]
Patch

(In reply to Jared Wein [:jaws] from comment #12)
> How come we don't need to check if the event is trusted anymore?

Because I'm an idiot (and also, because it was throwing security exceptions). I'll come up with a better patch after our meeting. Thanks to bholley and bz for helping out in figuring out how to deal with this.

Comment 14

[:Gijs (he/him)]

Attachment: Patch v2

Here's a better patch that keeps checking isTrusted. Tested on trunk and beta (will test aurora in a second).

I've left out the refactor that was in the original patch... we could probably do with just one menuitem that calls a toggle function and whose label is determined at runtime (rather than 2 menuitems which hide/show depending on the same check). That'd simplify the code a little. However, it's not *necessary* to do that now to fix this bug and I wanted to minimize risk for beta.

Comment 15

[:Gijs (he/him)]

Attachment: Aurora patch

Comment 16

[:Gijs (he/him)]

Attachment: Beta patch

These patches are really the same, so not asking for separate review, but nsContextMenu.js has so much churn that the patch up for review doesn't apply cleanly. These do.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 738145 [details] [diff] [review]
Beta patch

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

What is the difference between the three patches? Is it just the diff offsets?

The code changes look fine here, but I'll defer to release management if we should take this patch or one that just hides the context menuitems for Beta.

Comment 18

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #17)
> Comment on attachment 738145 [details] [diff] [review]
> Beta patch
> 
> Review of attachment 738145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What is the difference between the three patches? Is it just the diff
> offsets?

No, it's just diff contexts that changed. Here's a diff between the patches for beta and aurora, for instance: http://pastebin.mozilla.org/2311450 .

I do not believe any of the context changes are relevant for the patch itself.

> The code changes look fine here, but I'll defer to release management if we
> should take this patch or one that just hides the context menuitems for Beta.

Right, sure.

Comment 19

[:Gijs (he/him)]

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

Comment 20

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) from comment #11)
> Err, cross-posting from bug 862213, which appears to be closed now.
> 
> So, why does this expando need to live in content? Does content set it
> somewhere? It seems like it should just live on the Xray wrapper (accessible
> to the XBL code but not to content)...

Huh. I don't know why I didn't see this before. In principle that'd be a good idea, if the wrapper was guaranteed to persist... I think. Jared?


The reason we're using an expando at all is because the XBL binding itself can go away if the element gets hidden, and we want to persist the state of the statistics, AIUI.

Comment 21

[:Gijs (he/him)]

(reopening to get this back on radars, although there's a working patch on m-c...)

Comment 22

[bhavana bajaj [:bajaj]]

(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to Jared Wein [:jaws] from comment #17)
> > Comment on attachment 738145 [details] [diff] [review]
> > Beta patch
> > 
> > Review of attachment 738145 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > What is the difference between the three patches? Is it just the diff
> > offsets?
> 
> No, it's just diff contexts that changed. Here's a diff between the patches
> for beta and aurora, for instance: http://pastebin.mozilla.org/2311450 .
> 
> I do not believe any of the context changes are relevant for the patch
> itself.
> 
> > The code changes look fine here, but I'll defer to release management if we
> > should take this patch or one that just hides the context menuitems for Beta.
> 
> Right, sure.

Please make sure to explain the risk vs user impact in either approach, which will he helpful to us. Given, we are in the second half of the Beta cycle a low-risk work around is much preferred .

Comment 23

[Philip Chee]

If you're going to rewrite this patch can I suggest that you use the DOM4 CustomEvent constructor? q.v. <https://developer.mozilla.org/en-US/docs/DOM/Event/CustomEvent>

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 738135 [details] [diff] [review]
Patch v2

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

Oh, I see. You can't use an Xray expando because those are per-origin, and this code expects the property to be visible by both the XBL and chrome, which are running with different principals.

The fact that this is necessary is kind of indicative of a design problem IMO, but I'll withdraw my objections for now.

Comment 25

[:Gijs (he/him)]

Per comment #24, marking fixed.

Comment 26

[:Gijs (he/him)]

Comment on attachment 738143 [details] [diff] [review]
Aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 834697
User impact if declined: The Show Statistics/Hide Statistics context menu items for HTML5 <video> will be completely broken
Testing completed (on m-c, etc.): Been on m-c since yesterday, was tested manually on an aurora build already.
Risk to taking this patch (and alternatives if risky): approximately 0. This patch only deals with this specific feature, and is extremely unlikely to break anything else. The feature as it is is completely broken on aurora, so it can essentially only get better.
String or IDL/UUID changes made by this patch: none

Comment 27

[:Gijs (he/him)]

Comment on attachment 738145 [details] [diff] [review]
Beta patch

[Approval Request Comment]
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 834697
User impact if declined: After showing the statistics once for a video, hiding the statistics is no longer possible.
Testing completed (on m-c, etc.): Been on m-c since yesterday, was tested manually on a beta build already.
Risk to taking this patch (and alternatives if risky): low, as the code touched for the fix is very limited in scope and should only affect this specific issue. If this is still deemed too risky, a 0-risk approach would be to comment out the menu items in the context menu file so as to disable the feature completely. That has the obvious negative result that web developers and/or other users who rely on this feature would not be able to use it during this cycle, and because the feature was previously released, that may be a strange experience.
String or IDL/UUID changes made by this patch: none

Comment 28

[:Gijs (he/him)]

Checked into aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/882d023c79f0

Comment 29

[bhavana bajaj [:bajaj]]

Requested, : gkw to help with verification on trunk or aurora to confirm the issue is fixed in preparation for beta uplift. Thanks for the help Gary :)

Comment 30

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Works great on Nightly 2013-04-17. I retested with the video on https://marketplace.firefox.com/developers/partners as well as some other .webm videos.

Comment 31

[bhavana bajaj [:bajaj]]

(In reply to :Gijs Kruitbosch from comment #27)
> Comment on attachment 738145 [details] [diff] [review]
> Beta patch
> 
> [Approval Request Comment]
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 834697
> User impact if declined: After showing the statistics once for a video,
> hiding the statistics is no longer possible.
> Testing completed (on m-c, etc.): Been on m-c since yesterday, was tested
> manually on a beta build already.
> Risk to taking this patch (and alternatives if risky): low, as the code
> touched for the fix is very limited in scope and should only affect this
> specific issue. If this is still deemed too risky, a 0-risk approach would
> be to comment out the menu items in the context menu file so as to disable
> the feature completely. That has the obvious negative result that web
> developers and/or other users who rely on this feature would not be able to
> use it during this cycle, and because the feature was previously released,
> that may be a strange experience.
> String or IDL/UUID changes made by this patch: none

With the verification we have and because this is a FX21 regression approving the low-risk self contained patch for Beta 4.Please look out for any regressions or comment in the bug if there is any additional testing needed here to verify context menus etc in general on the web .Avoiding the alternate approach as it does not sound like a good user experience.

Comment 32

[:Gijs (he/him)]

Now fixed on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/1d78a56005f1 . Thanks folks!

Comment 33

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on FF 21 beta 4 (build ID: 20130423212553)

User agents:
Mozilla/5.0 (Windows NT 5.1; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0

Comment 34

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on FF 22 beta 2 (build ID: 20130521223249)

User agents:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (X11; Linux i686; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0