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

VERIFIED FIXED

Status

()

Toolkit
Video/Audio Controls
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: gkw, Assigned: Gijs)

Tracking

({regression})

21 Branch
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20 unaffected, firefox21+ verified, firefox22+ verified, firefox23 verified)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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.
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
Duplicate of this bug: 849054
Pretty sure this used to work.
Keywords: regression
(Reporter)

Updated

4 years ago
Keywords: regressionwindow-wanted
Last good nightly: 2013-02-14
First bad nightly: 2013-02-15

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aceeea086ccb&tochange=953b1db7a246
Keywords: regressionwindow-wanted
Likely cause:
Bobby Holley — Bug 834697 - Enable XBL scopes, and disable assertion. r=bz,me
Blocks: 834697
status-firefox20: --- → unaffected
status-firefox21: --- → affected
status-firefox22: --- → affected
OS: Mac OS X → All
Hardware: x86 → All
Version: Trunk → 21 Branch
jaws, if you can point me to the line of video code that appears to be malfunctioning, I can almost certain explain why. :-)
(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
Flags: needinfo?(bobbyholley+bmo)
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.
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Updated

4 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
tracking-firefox21: --- → ?
tracking-firefox22: --- → ?
(Assignee)

Updated

4 years ago
Duplicate of this bug: 862213
(Assignee)

Comment 9

4 years ago
Created attachment 737833 [details] [diff] [review]
Patch

This fixes the menu being completely broken (trunk, aurora) and halfway broken (beta).
Attachment #737833 - Flags: review?(jaws)
Attachment #737833 - Flags: feedback?(bobbyholley+bmo)
(Assignee)

Comment 10

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

https://tbpl.mozilla.org/?tree=Try&rev=846afe30bcbe
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 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?
(Assignee)

Comment 13

4 years ago
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.
Attachment #737833 - Attachment is obsolete: true
Attachment #737833 - Flags: review?(jaws)
Attachment #737833 - Flags: feedback?(bobbyholley+bmo)
(Assignee)

Comment 14

4 years ago
Created attachment 738135 [details] [diff] [review]
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.
Attachment #738135 - Flags: review?(jaws)
(Assignee)

Comment 15

4 years ago
Created attachment 738143 [details] [diff] [review]
Aurora patch
(Assignee)

Comment 16

4 years ago
Created attachment 738145 [details] [diff] [review]
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.
Attachment #738135 - Flags: review?(jaws) → review+
Attachment #738143 - Flags: review+
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.
Attachment #738145 - Flags: review+
(Assignee)

Comment 18

4 years ago
(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.
(Assignee)

Comment 19

4 years ago
https://hg.mozilla.org/mozilla-central/rev/f90420b412c0
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox23: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 20

4 years ago
(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.
(Assignee)

Updated

4 years ago
Attachment #738135 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 21

4 years ago
(reopening to get this back on radars, although there's a working patch on m-c...)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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 .
tracking-firefox21: ? → +
tracking-firefox22: ? → +

Comment 23

4 years ago
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 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.
Attachment #738135 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 25

4 years ago
Per comment #24, marking fixed.
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 26

4 years ago
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
Attachment #738143 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 27

4 years ago
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
Attachment #738145 - Flags: approval-mozilla-beta?

Updated

4 years ago
Attachment #738143 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 28

4 years ago
Checked into aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/882d023c79f0
status-firefox22: affected → fixed

Updated

4 years ago
Blocks: 863008
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 :)
(Reporter)

Comment 30

4 years ago
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.
Status: RESOLVED → VERIFIED
(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.

Updated

4 years ago
Attachment #738145 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 32

4 years ago
Now fixed on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/1d78a56005f1 . Thanks folks!
status-firefox21: affected → fixed
status-firefox23: fixed → verified
QA Contact: cornel.ionce
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
status-firefox21: fixed → verified
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
status-firefox22: fixed → verified
You need to log in before you can comment on or make changes to this bug.