915137 - Context menu broken with detail.contextmenu is null

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Browser, defect)

Description

[Dale Harvey (:daleharvey)]

E/GeckoConsole(  122): [JavaScript Error: "detail.contextmenu is null" {file: "app://system.gaiamobile.org/js/context_menu.js" line: 13}]

This error is received in the system app, which means the contextmenu event gets sent to the system app (which it shouldnt be), it looks like the browser after the first time doesnt receive a context menu event

Comment 1

[Dale Harvey (:daleharvey)]

May possibly be related to https://bugzilla.mozilla.org/show_bug.cgi?id=911195

Comment 3

[Tony Chung [:tchung]]

i can reproduce this 100%, and my steps are in the dupe bug, 915875.   however, you can get out of this js error by force closing the browser app and relaunching the video content.   then it returns.  but once you save video once, it seems you can't save video anymore until you do the force kill browser thing.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Naoki, let's get a regression window here

Comment 7

[Lucas Aliaga]

I was initially able to reproduce this issue on 9/16 (and 9/12) Buri 1.2 build. While searching for the regression window I flashed builds all the way back up to the current from 9/1, and now I am unable to repro consistently. The context menu does appear on 9/16.

My theory is that by performing normal gestures, after a certain point the intended gesture "long tap" isn't recognized because it's getting confused with a swipe or drag gesture. Once the intent gets confused, this issue will repro 100%.

Environmental Variables
Build ID: 20130916040205
Gecko: http://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9
Gaia: a0079597d510ce8ea0b9cbb02c506030510b9eeb
Platform Version: 26.0a1

I am exploring this issue further for a solid repro.

Comment 8

[Lucas Aliaga]

Found the regression window. Context menu becomes unresponsive after following one of the options (e.g. Save Video on Youtube) starting on the 9/4 build.

- Working 9/3 Build -
Environmental Variables
Build ID: 20130903040204
Gecko: http://hg.mozilla.org/mozilla-central/rev/d54e0cce6c17
Gaia: c6d58088f207829d96e7bb33ddacf990e90752fb
Platform Version: 26.0a1

- Broken 9/4 Build -
Environmental Variables
Build ID: 20130904040205
Gecko: http://hg.mozilla.org/mozilla-central/rev/7ff96bd19c1c
Gaia: b6c5bf1d24230bfed4a8f680e625fa2175001f82
Platform Version: 26.0a1

Comment 9

[Jason Smith [:jsmith]]

Looks like this is a regression from bug 907056.

Comment 10

[Dale Harvey (:daleharvey)]

Not entirely sure that can cause it, as we arent actually recieving the contextmenu event inside the browser

Also the fact that the system gets the context menu is a bug, it may be the same bug, looking into it now

Comment 11

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/57230822

Comment 12

[Dale Harvey (:daleharvey)]

Apologies, I am stuck on this bug

So the visible effect on this bug is nothing, context menus continue to work inside the browser, when they dont it s a seperate issue, however I think the symptoms show a fairly serious error that could lead to a lot of confusing bugs

When I long press on a link inside browser content, I receive a contextmenu event inside the browserElementChild of app://browser.gaiamobile.org/index.html, I shouldnt ever get this event, this gets bubbled up to system and triggers the detail.contextmenu is null error, this event is fired from nsEventStateManager

After that I get a contextmenu event triggered from tabChild in the browserElementChild of the webcontent, this is the one I expect and it gets bubbled into mozbrowser where the browser shows the context menu UI

I/Gecko   (  674): nsEventStateManager::FireContextClick outer
I/Gecko   (  674): BrowserElementChildPreload - Got contextmenu from app://browser.gaiamobile.org/index.html
I/Gecko   (  674): BrowserElementParent.jsm - fireCtxMenuEventFromMsg: contextmenu {"systemTargets":[],"contextmenu":null,"msg_name":"contextmenu"}
I/Gecko   (  674): BrowserElementParent.jsm - into app://browser.gaiamobile.org/manifest.webapp
E/GeckoConsole(  674): [JavaScript Error: "TypeError: detail.contextmenu is null" {file: "app://system.gaiamobile.org/js/context_menu.js" line: 13}]
I/Gecko   (  674): BrowserElementChildPreload - Got contextmenu from app://system.gaiamobile.org/index.html
I/Gecko   (  674): BrowserElementParent.jsm - fireCtxMenuEventFromMsg: contextmenu {"systemTargets":[],"contextmenu":null,"msg_name":"contextmenu"}
I/Gecko   (  674): BrowserElementParent.jsm - into app://system.gaiamobile.org/manifest.webapp
I/Gecko   (  847): TabChild::RecvHandleLongTap
I/Gecko   (  847): nsDOMWindowUtils::SendMouseEventCommon contextmenu
I/Gecko   (  847): nsPresShell::HandleEventInternal NS_CONTEXTMENU
I/Gecko   (  847): BrowserElementChildPreload - Got contextmenu from http://www.google.com/search?q=r
I/Gecko   (  674): BrowserElementParent.jsm - fireCtxMenuEventFromMsg: contextmenu {"systemTargets":[{"nodeName":"A","data":{"uri":"http://www.google.com/url?q=http://www.r-project.org/&sa=U&ei=X8VKUv2TFa-k0AXp_YGYAg&ved=0CAwQFjAA&usg=AFQjCNGQshRHRBeLvRWqy9OO-ujsXB241g"}}],"contextmenu":null,"msg_name":"contextmenu"}
I/Gecko   (  674): BrowserElementParent.jsm - into
E/GeckoConsole(  674): Content JS LOG at app://browser.gaiamobile.org/js/browser.js:430 in browser_handleBrowserEvent/<: !! mozbrowsercontextmenu
E/GeckoConsole(  674): Content JS LOG at app://browser.gaiamobile.org/js/browser.js:431 in browser_handleBrowserEvent/<: {"systemTargets":[{"nodeName":"A","data":{"uri":"http://www.google.com/url?q=http://www.r-project.org/&sa=U&ei=X8VKUv2TFa-k0AXp_YGYAg&ved=0CAwQFjAA&usg=AFQjCNGQshRHRBeLvRWqy9OO-ujsXB241g"}}],"contextmenu":null,"msg_name":"contextmenu"}

kats

I/Gecko   (  674): nsEventStateManager::FireContextClick outer
I/Gecko   (  674): BrowserElementChildPreload - Got contextmenu from app://browser.gaiamobile.org/index.html

Shouldnt happen, events that happen inside subframes shouldnt be seen in the system app at all, let along before the mozbrowser sees them right? (browserElement will bubble these events back up manually, but this happens way before that), any suggestions on what would cause it / how to fix it?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Sorry, there's not enough info here to let me help you. Can you stick a breakpoint in "nsEventStateManager::FireContextClick outer" print location and get a backtrace? That should indicate where the event is coming from.

Comment 14

[Dale Harvey (:daleharvey)]

Apologies for the delay, but getting up and running with gdb is gonna be handy in future 

Breakpoint 1, nsEventStateManager::FireContextClick (this=0x46d66660) at /Users/daleharvey/src/moz/mozilla-central-git/content/events/src/nsEventStateManager.cpp:1666
1666	  printf_stderr("nsEventStateManager::FireContextClick outer");
(gdb) bt
#0  nsEventStateManager::FireContextClick (this=0x46d66660) at /Users/daleharvey/src/moz/mozilla-central-git/content/events/src/nsEventStateManager.cpp:1666
#1  0x412c4490 in nsEventStateManager::sClickHoldCallback (aTimer=<value optimized out>, aESM=0x46d66660)
    at /Users/daleharvey/src/moz/mozilla-central-git/content/events/src/nsEventStateManager.cpp:1641
#2  0x41cff6a8 in nsTimerImpl::Fire (this=0x451b6920) at /Users/daleharvey/src/moz/mozilla-central-git/xpcom/threads/nsTimerImpl.cpp:546
#3  0x41cff8a2 in nsTimerEvent::Run (this=0x45f58570) at /Users/daleharvey/src/moz/mozilla-central-git/xpcom/threads/nsTimerImpl.cpp:630
#4  0x41cfb634 in nsThread::ProcessNextEvent (this=0x403024e0, mayWait=<value optimized out>, result=0xbe9ba7b7)
    at /Users/daleharvey/src/moz/mozilla-central-git/xpcom/threads/nsThread.cpp:622
#5  0x41cc4202 in NS_ProcessNextEvent (thread=0x403024e0, mayWait=true) at /Users/daleharvey/src/moz/mozilla-central-git/xpcom/glue/nsThreadUtils.cpp:238
#6  0x41988ace in mozilla::ipc::MessagePump::Run (this=0x40301d30, aDelegate=0x4033b0c0) at /Users/daleharvey/src/moz/mozilla-central-git/ipc/glue/MessagePump.cpp:116
#7  0x41d25dfa in MessageLoop::RunInternal (this=0x4033b0c0) at /Users/daleharvey/src/moz/mozilla-central-git/ipc/chromium/src/base/message_loop.cc:220
#8  0x41d25e12 in MessageLoop::RunHandler (this=0x4033b0c0) at /Users/daleharvey/src/moz/mozilla-central-git/ipc/chromium/src/base/message_loop.cc:213
#9  MessageLoop::Run (this=0x4033b0c0) at /Users/daleharvey/src/moz/mozilla-central-git/ipc/chromium/src/base/message_loop.cc:187
#10 0x4190a94a in nsBaseAppShell::Run (this=0x44835700) at /Users/daleharvey/src/moz/mozilla-central-git/widget/xpwidgets/nsBaseAppShell.cpp:161
#11 0x4181f936 in nsAppStartup::Run (this=0x44772c40) at /Users/daleharvey/src/moz/mozilla-central-git/toolkit/components/startup/nsAppStartup.cpp:269
#12 0x40d887a4 in XREMain::XRE_mainRun (this=0xbe9baa04) at /Users/daleharvey/src/moz/mozilla-central-git/toolkit/xre/nsAppRunner.cpp:3868
#13 0x40d8b542 in XREMain::XRE_main (this=0xbe9baa04, argc=<value optimized out>, argv=<value optimized out>, aAppData=0x22170)
    at /Users/daleharvey/src/moz/mozilla-central-git/toolkit/xre/nsAppRunner.cpp:3936
#14 0x40d8b6bc in XRE_main (argc=1, argv=0xbe9bcbf4, aAppData=0x22170, aFlags=<value optimized out>)
    at /Users/daleharvey/src/moz/mozilla-central-git/toolkit/xre/nsAppRunner.cpp:4138
#15 0x00009a34 in do_main (argc=1, argv=0xbe9bcbf4) at /Users/daleharvey/src/moz/mozilla-central-git/b2g/app/nsBrowserApp.cpp:168
#16 main (argc=1, argv=0xbe9bcbf4) at /Users/daleharvey/src/moz/mozilla-central-git/b2g/app/nsBrowserApp.cpp:261

is the backtrace from nsEventStateManager::FireContextClick

Comment 15

[Kartikaya Gupta (email:[e-mail])]

I asked Dale to get another backtrace from the code that posts the sClickHoldCallback in the backtrace above, and it pointed to the nsWindow::DispatchEvent at gonk/nsWindow.cpp:484.

So basically, this is part of the normal event flow in the B2G browser. That is, (1) gonk receives touch events, (2) sends them to Gecko for hit testing to see if they are in the content area or the root process area, (3) sends them to the APZC because they are in the content area, which also then (4) sends them to TabChild for processing in the child.

Step (2) above apparently has its own long-press gesture detection which runs in the root process, and step (3) also does long-press gesture detection which notifies the child process directly, so you end up with two long press events, one in the root process and one in the child.

I suspect this will be fixed by the patches I'm working on in bug 920036 but I don't know how far those are from landing because I keep running into other problems and distractions. Smaug, is there some way to turn off the long-press gesture detection in the root process if the events are destined for the child process? That seems wrong regardless of whether or not APZC is in the picture.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

If it is possible to set prefs per process type, then it is possible to
not set ui.click_hold_context_menus.delay on system process. But I guess in some cases we want the
contextmenu event on system process?
We could change the behavior of nsEventStateManager::CreateClickHoldTimer.
Check there whether mGestureDownContent is an IsRemoteTarget(), and if so, return early.

Comment 17

[Dale Harvey (:daleharvey)]

We do want contextmenus in the system process, at the least we use them in the browser app which is in process, I would expect they are used inside various notifications etc as well

  if (IsRemoteTarget(mGestureDownContent))
    return;

in CreateClickHoldTimer does prevent this error, I still see the same error when long pressing inside the browser app itself (in process) however it looks like that may be caused by incorrect event bubbling done by browserElement.

Are there any other tests for similiar functionality (the first part with the child events)? can start attempting to write a test for this

Will look into the bubbling problem seperately

Comment 18

[Dale Harvey (:daleharvey)]

I split the above into a seperate bug, it has similiar consequences but different cause (https://bugzilla.mozilla.org/show_bug.cgi?id=923081), with that patch apply we still get a contextmenu in the system app when we long press on application content (I tested on a button inside the sms app)

http://pastebin.mozilla.org/3183021

This is a backtrace from breaking in https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/context_menu.js#L11

Strangely this is getting fired, but there is no mozbrowsercontextmenu event being sent into the system app, only the sms one (https://github.com/mozilla/mozilla-central/blob/master/dom/browser-element/BrowserElementParent.jsm#L338)

I/Gecko   ( 3835): TabChild::FireContextMenuEvent
I/Gecko   ( 3835): nsDOMWindowUtils::SendMouseEventCommon contextmenu
I/Gecko   ( 3835): nsPresShell::HandleEventInternal NS_CONTEXTMENU
I/Gecko   ( 3835): BrowserElementChildPreload - Got contextmenu from app://sms.gaiamobile.org/index.html
I/Gecko   ( 3628): BrowserElementParent.jsm - fireCtxMenuEventFromMsg: contextmenu {"systemTargets":[{"nodeName":"A","data":{"uri":"app://sms.gaiamobile.org/index.html#new"}}],"contextmenu":null,"msg_name":"contextmenu"}
I/Gecko   ( 3628): BrowserElementParent.jsm - into app://sms.gaiamobile.org/manifest.webapp

Comment 19

[Dale Harvey (:daleharvey)]

So I got myself confused here due to the other issue, we should be expecting mozbrowser events in the system app when we long press in the sms app, all that need to be clarified is why detail.contextmenu is null despite there being a check on it in BrowserElementParent

https://github.com/mozilla/mozilla-central/blob/master/dom/browser-element/BrowserElementParent.jsm#L345

Comment 20

[Dale Harvey (:daleharvey)]

The check doesnt stop the event being sent, its supposed to since the contextmenu property only contains extended data that is optional, simple gaia fix

Comment 21

[Dale Harvey (:daleharvey)]

Attachment: Dont error if custom contextmenu data is not defined

This doesnt cause any use visible bugs so cant really test it, but will stop a lot of confusing log messages.

Comment 22

[Gregor Wagner [:gwagner]]

Comment on attachment 813153 [details] [review]
Dont error if custom contextmenu data is not defined

Thanks!

Comment 23

[Dale Harvey (:daleharvey)]

Cheers

https://github.com/mozilla-b2g/gaia/commit/9f9898d5b32caa2e8c85fc5abec9ec16127d3dd6

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 9f9898d5b32caa2e8c85fc5abec9ec16127d3dd6 to:
v1.2: d6f28f9611f7be3e6d3d10ffea0d61125d439193