Description: The user is unable to open a YouTube video into Full Screen using the player control, if the Browser app has been opened previously. The first time browser is opened, Full Screen will work properly, but every subsequent time Browser is launched, Full Screen will no longer work. This persists after a device reboot. Note: All other player controls work properly (scrubbing, mute, pause/play) Repro Steps: 1) Update a Flame to 20150204010225 2) Connect to a WiFi or Data network. 3) Open the Browser app, and navigate to YouTube.com 4) Select a video and begin video playback. 5) Tap the video to bring up the player controls, then tap the Full Screen icon. 6) Exit Full Screen, long press the Home Button, and close the Browser from Card View 7) Re-open Browser, and navigate back to the same video and begin video playback. 8) Tap the video to bring up the player controls, then tap the Full Screen icon. Actual: The video does not open to Full Screen the second time, or any subsequent time. Expected: The video always opens to Full Screen properly. Environmental Variables: Device: Flame 3.0 (319MB)(Full Flash) Build ID: 20150204010225 Gaia: dfebaaa8aab43470f482d09d71137bab840c3ae9 Gecko: 0c2f7434c325 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 38.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0 Repro frequency: 8/8 Link to failed test case: https://moztrap.mozilla.org/manage/case/6073/ See attached: video clip (URL), logcat ----------------------------------------------- This issue does NOT occur on Flame 2.2. The video always opens to Full Screen properly. Environmental Variables: Device: Flame 2.2 (319MB)(Full Flash) Build ID: 20150204002509 Gaia: a4c4cc86303a554facb8f45b7e764e5c4473c3de Gecko: 8669c26fd4a5 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 37.0a2 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

[Blocking Requested - why for this release]: Functional regression that can fail smoke tests. Requesting a window.

Regression Window: Last working Mozilla-Inbound build: Device: Flame 3.0 Master BuildID: 20150201170935 Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a Gecko: 231a8c61b49f Version: 38.0a1 (3.0) Firmware: V18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0 First broken Mozilla-Inbound build: Device: Flame 3.0 Master BuildID: 20150201174135 Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a Gecko: bcefc7d8d885 Version: 38.0a1 (3.0 Master) Firmware: V18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0 Working Gaia with Broken Gecko issue DOES occur: Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a Gecko: bcefc7d8d885 Working Gecko with Broken Gaia issue does NOT occur: Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a Gecko: 231a8c61b49f Mozilla-Inbound Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=231a8c61b49f&tochange=bcefc7d8d885 Issue appears to be caused by bug 950934

:benfrancis, this is a smoketest blocker and we need the offending commit to be backed out asap.

Kats or Botond can you take a look here, patch for bug 950934 may have introduced this smoketest blocker.

:kats, could you please take a look?

(In reply to Martin Shuman [:Marty] from comment #0) > Actual: > The video does not open to Full Screen the second time, or any subsequent > time. > For the record you can make it work again by starting the browser, going back to the app switcher, and then selecting the browser again. The fullscreen button is getting clicked just fine, but the fullscreen request is being denied with this in the log: [JavaScript Warning: "Request for full-screen was denied because requesting element is not in the currently focused tab." {file: "http://m.youtube.com/watch?v=kO5n3qWYVYg" line: 0}]

The backout is not trivial to do since a bunch of other stuff has landed on top of it since then. I pushed my attempt at backing it out to https://treeherder.mozilla.org/#/jobs?repo=try&revision=b239b420dc4e. It would be great if QA could verify on that try build that stuff works again and we don't end up backing this out for no reason. In the meantime I'll see if I can come up with a fix for this bug.

if we backout 950934 we will hit sanity blocker bug 1128566.

(In reply to Peter Bylenga [:PBylenga] from comment #8) > if we backout 950934 we will hit sanity blocker bug 1128566. The backout also includes relevant parts of bug 1005815. Bug 1128566 is caused by having bug 1005815 without bug 950934, so backing out both will avoid it.

Okay excellent :). Putting QAWanted to test the try in Comment 7 when it's done to see if it fixes this issue and doesn't introduce a worst state.

Spoke offline with :kats and given the complicated backout, we'll let him help investigate and instead focus on the forward fix *fingers crossed* ;) We'll monitor this till tomorrow to reevaluate any calls. Note to QA, We *will* hit the same issue in tomorrow's build and lets hope to have the smoketest report include the update this bug will have tomorrow. Thanks!

Following up from #gaia/IRC discussion, there is a slight difference in how we switch to an app via the edge gesture vs. the task manager. In edge gesture, we wind up at https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L1882, which transitions the appWindow to 'opened' state (implemented in AppTransitionController). In task manager we call appWindow.open() which goes via the 'opening' state. This may or may not be significant, but the different code paths do exist.

After much debugging, I think I diagnosed the problem, though I'm not sure how to fix it. We were relying on the piece of code in gonk/nsWindow.cpp that dispatches a mouse event to parent-process Gecko in response to a touch-start [1] to run even if the touch-start was targetting a child process. Bugs 1005815 and 950934 moved this dispatch to be done by ChromeProcessController, but only if the event targets the parent process. The touch-start that appears to need this, is the one for the tap on the initial page of the Browser app (the one with the Top Sites / History) that opens the Youtube page (which runs in a new Browser app). This dispatch would call nsFocusManager::SetFocus, still in the parent process, via the following stack: #0 nsFocusManager::SetFocus (this=0xb2761840, aElement=0xad8edac8, aFlags=4098) at /home/botond/dev/mozilla/central/dom/base/nsFocusManager.cpp:471 #1 0xb4c7689c in mozilla::EventStateManager::PostHandleEvent (this=this@entry=0xae12b7c0, aPresContext=aPresContext@entry=0xaec60000, aEvent=aEvent@entry=0xbeca96d0, aTargetFrame=<optimized out>, aStatus=aStatus@entry=0xbeca9618) at ../../../dom/events/EventStateManager.cpp:2893 #2 0xb51d34b2 in PresShell::HandleEventInternal (this=this@entry=0xaf9f4840, aEvent=aEvent@entry=0xbeca96d0, aStatus=aStatus@entry=0xbeca9618) at ../../../layout/base/nsPresShell.cpp:8258 #3 0xb51d36b8 in PresShell::HandlePositionedEvent (this=this@entry=0xaf9f4840, aTargetFrame=aTargetFrame@entry=0xaec17680, aEvent=aEvent@entry=0xbeca96d0, aEventStatus=aEventStatus@entry=0xbeca9618) at ../../../layout/base/nsPresShell.cpp:7949 #4 0xb51d428c in PresShell::HandleEvent (this=0xb25a79c0, aFrame=<optimized out>, aEvent=0xbeca96d0, aDontRetargetEvents=<optimized out>, aEventStatus=0xbeca9618) at ../../../layout/base/nsPresShell.cpp:7746 #5 0xb5038bfc in nsViewManager::DispatchEvent (this=<optimized out>, aEvent=aEvent@entry=0xbeca96d0, aView=aView@entry=0xb1de5330, aStatus=aStatus@entry=0xbeca9618) at ../../view/nsViewManager.cpp:774 #6 0xb5036838 in nsView::HandleEvent (this=<optimized out>, aEvent=0xbeca96d0, aUseAttachedEvents=<optimized out>) at ../../view/nsView.cpp:1097 #7 0xb50603bc in nsWindow::DispatchEvent (this=0xb11d9600, aEvent=<optimized out>, aStatus=@0xbeca964c: nsEventStatus_eConsumeNoDefault) at ../../../widget/gonk/nsWindow.cpp:555 #8 0xb50614f6 in nsWindow::DispatchTouchInputViaAPZ (this=0xb11d9600, aInput=...) at ../../../widget/gonk/nsWindow.cpp:298 nsFocusManager::SetFocus would then kick off the following chain: nsFocusManager::SetFocusInner nsFocusManager::Focus TabParent::Activate ---------------------------- process boundary TabChild::RecvActivate nsWebBrowser::Activate nsFocusManager::WindowRaised The WindowRaised call sets mActiveWindow [2] in the Browser process. Without this dispatch, mActiveWindow does not get set in the Browser process, which causes IsInActiveTab() to return false [3], which causes the full-screen request to be denied [4]. =================================== I note that for other taps that launch a new process, for example the tap on the Homescreen that launches the Browser app, the nsFocusManager::SetFocus call in the parent process that triggers the WindowRaised call in the newly activated child, happens via a different route, from JS: 0 bm_focus() [\"app://system.gaiamobile.org/js/browser_mixin.js\":150] this = [object Object] 1 AppWindowManager.prototype.focus() [\"app://system.gaiamobile.org/js/app_window_manager.js\":56] this = [object Object] 2 AppWindowManager.prototype.setHierarchy(active = true) [\"app://system.gaiamobile.org/js/app_window_manager.js\":46] this = [object Object] 3 .updateHierarchy() [\"app://system.gaiamobile.org/js/hierarchy_manager.js\":110] this = [object Object] 4 .handleEvent(evt = [object CustomEvent]) [\"app://system.gaiamobile.org/js/hierarchy_manager.js\":152] this = [object Object] 5 awm_publish(event = \"appwindowmanager-activated\") [\"app://system.gaiamobile.org/js/app_window_manager.js\":806] this = [object Object] 6 awm__changeActiveApp(instanceID = \"AppWindow_2\") [\"app://system.gaiamobile.org/js/app_window_manager.js\":844] this = [object Object] 7 awm_handleEvent(evt = [object CustomEvent]) [\"app://system.gaiamobile.org/js/app_window_manager.js\":552] this = [object Object] 8 AppWindow.prototype.publish(event = \"opening\") [\"app://system.gaiamobile.org/js/app_window.js\":1269] this = [object Object] 9 atc_changeTransitionState(evt = \"open\") [\"app://system.gaiamobile.org/js/app_transition_controller.js\":110] this = [object Object] 10 AppTransitionController.prototype.requireOpen(animation = undefined) [\"app://system.gaiamobile.org/js/app_transition_controller.js\":303] this = [object Object] 11 aw_open(animation = undefined) [\"app://system.gaiamobile.org/js/app_window.js\":1797] this = [object Object] 12 awm_switchApp/<() [\"app://system.gaiamobile.org/js/app_window_manager.js\":278] this = [object Object] (I'll skip over the part where I rant about how difficult it was to get this JS trace on B2G.) I'm not sure whether it's expected that this Gaia path is triggered during the "Homescreen -> Browser app" switch, but not during the "first Browser app (with Top Sites / History) -> second Browser app (with YouTube)" switch. [1] http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp?rev=5af5c02018a2#298 [2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp?rev=89290481e882#695 [3] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=c85ab430a0c2#11505 [4] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=c85ab430a0c2#11596

(In reply to Botond Ballo [:botond] from comment #13) > I'm not sure whether it's expected that this Gaia path is triggered during > the "Homescreen -> Browser app" switch, but not during the "first Browser > app (with Top Sites / History) -> second Browser app (with YouTube)" switch. :alive, perhaps you can comment on this? (See the previous comment, from "=========" downwards).

It seems like a focus issue or something. After doing the repor steps, I can get the full screen button working again by focusing the search/url bar and closing again.

:alive, specifically what we would like to happen is that when the user clicks on something from the topsites/history screen at [1] the new appwindow that is spawned should have .open() called on it, or some equivalent so that it is given focus within gecko. [1] https://github.com/mozilla-b2g/gaia/blob/18b38bc3dc6f838500b88820c05589438b90e62c/apps/search/js/providers/places.js#L108

This sounds like another bug we hit recently. See https://bugzilla.mozilla.org/show_bug.cgi?id=1128053 The problem there is to say: If the user clicks some button in system app(will steal the focus from the app), and then back to the app to click something else, the value selector will not be triggered again - and no touch events are forwarded to the app. IMO, no matter if system app calls iframe.focus() correctly at correct timing, when the user taps anything inside the app, the "outer focus" should be delivered to the app. System app is not doing focus management in the past, and I don't think it has enough information to do that. I don't know why bug 950934 changes the behavior; if we really needs system app to take the role of focus management we have to change a lot :/

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #17) > This sounds like another bug we hit recently. See > https://bugzilla.mozilla.org/show_bug.cgi?id=1128053 > > The problem there is to say: If the user clicks some button in system > app(will steal the focus from the app), and then back to the app to click > something else, the value selector will not be triggered again - and no > touch events are forwarded to the app. > > IMO, no matter if system app calls iframe.focus() correctly at correct > timing, when the user taps anything inside the app, the "outer focus" should > be delivered to the app. System app is not doing focus management in the > past, and I don't think it has enough information to do that. > > I don't know why bug 950934 changes the behavior; if we really needs system > app to take the role of focus management we have to change a lot :/ So my question: is it possible for gecko to change the focus window when the user taps anything inside the window? If not, we will need to do a lot |iframe.focus()| call than we didn't do it before, because there are tons of cases the user clicks something outside the app :(

Thanks for the info, Alive! Given what you said, I agree it probably makes more sense to fix this in Gecko and to make sure that the right app gets focused when it's clicked on. The attached patch should do this; with local testing it seems to fix this Youtube bug.

Also, try push with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3c277b9fab8

Comment on attachment 8559785 [details] [diff] [review] Patch Review of attachment 8559785 [details] [diff] [review]: ----------------------------------------------------------------- LGTM! I feel it wouldn't hurt for someone more familiar with the nsIFocusManager flags to verify that we're using appropriate ones, but given that this is a smoketest regression, I'm happy to have that done after the fact.

Blocked from testing this by bug https://bugzilla.mozilla.org/show_bug.cgi?id=1130086 B2G-Inbound build 20150206005126 is affected by this bug, so I will check this issue when 1130086 is fixed.

This got merged a few days ago and got missed. https://hg.mozilla.org/mozilla-central/rev/d26e94b94bcb

This issue is verified fixed on Flame 3.0 after 10 attempts The user is able to open videos on youtube and allow them to be full screen Environmental Variables: Device: Flame 3.0 (319mb)(Kitkat)(Full Flash) Build ID: 20150209010211 Gaia: 0d7b35f23402c4cb29bca6b98280fec48a196dec Gecko: 3436787a82d0 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 38.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Comment on attachment 8559785 [details] [diff] [review] Patch NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): required for parent-process APZ (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm tracking the full set of bugs that will need uplifting together at http://mzl.la/1zvKgmk; requesting approval on bugs individually but will wait until everything has approval before I uplift any of it. User impact if declined: can't have parent-process APZ or input-thread Testing completed: on master. some of these bugs have been baking for a while; others are more recent. Risk to taking this patch (and alternatives if risky): there's likely some edge cases that we haven't run into yet and will be uncovered with further testing. Bug 1128887 is the only known issue that we don't yet have a fix for but I consider that relatively minor and something we can fix after uplifting everything else. String or UUID changes made by this patch: none