Bug 37291 – Converting DOM timestamp -> X timestamp in plugin message is wrong

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Plug-ins

[Evan Martin]

When sending a message to a plugin via NPAPI, we need fill in the X event time as one of the fields of the struct.

In the Chromium, GTK, and Qt ports, the code looks like:
    xbutton.time = event->timeStamp();
But event->timeStamp() is the DOM timestamp of the event, not the X server time!  The X time is an opaque value which in actuality appears to be milliseconds since the server started.

This X event time is eventually propagated into the plugin which might use it to e.g. do a pointer grab, so providing the wrong time can cause grabs to fail (causing menus to not pop up) as well as race conditions.

[Evan Martin]

CC'ing some people who have touched these files recently.

[Kenneth Rohde Christiansen]

We should fix that. Any link to code getting the timestamp from the dom?

I will look into it tomorrow.

[Evan Martin]

Oh, and to disclaim my biases here: this bug is why context menus don't work in Linux Chrome.
  http://code.google.com/p/chromium/issues/detail?id=40157

It's more complicated in Chrome because the input event goes from browser process -> IPC -> renderer process -> webkit -> renderer process -> IPC -> plugin.  So it might be that you get good luck with the timing when you are a single-process browser.  If context menus work for you on
  http://www.communitymx.com/content/source/E5141/wmodetrans.htm
then this may not be release-critical.

[Evan Martin]

I'm only familiar with GTK, but in that case the GdkEvent* carries the X server time.  I guess we'd need to plumb that through the WebKit MouseEvent structure such that we have that time when we're constructing the XButtonEvent for the plugin (grep the code for XButtonEvent to find it).

I noticed we have a PlatformKeyboardEvent attached to the KeyboardEvent, so that might be a good place for it for keys; but for MouseEvent I don't see such a member.

[Kenneth Rohde Christiansen]

They don't :-). I should cc Girish, I think he was having problems getting that
to work.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> They don't :-). I should cc Girish, I think he was having problems getting that
> to work.

Getting the original X11 events with Qt is tricky. 

It is possible for the QWebView as I can reimplement the QWidget:x11Event(XEvent* event) and store the original event, but that won't work in the case of the QGraphicsWebView, which is becoming our most used view, as we cannot force our users to reimplement their QGraphicsView.

Also reimplementing QApplication is also no option. :-(

[Kenneth Rohde Christiansen]

Simon, what are our options here? Looking at the X11 code with Caio, it seems that one option would be to get the current QCoreApplication::EventFilter and use a special one calling the original one, using

EventFilter QCoreApplication::setEventFilter ( EventFilter filter )

I'm not very happy about this solution though.

[Evan Martin]

I managed to make things work for test_shell (a single-process browser using the Chromium backends) by passing 0 in for the time we give to the plugin (which is actually Xlib's "CurrentTime" constant).

That might be a good hack for you guys in the interim.  It didn't work for multi-process Chrome though (see the Chrome bug I linked for more detailed discussion).

[Simon Hausmann]

For Qt this should be easy to solve: For any incoming X event Qt remembers the value of the time variable in the X11 event and stores it in QX11Info::appTime(). It might be that QX11Info::appUserTime() is a slight "more" correct value.

[Kenneth Rohde Christiansen]

(In reply to comment #9)
> For Qt this should be easy to solve: For any incoming X event Qt remembers the
> value of the time variable in the X11 event and stores it in
> QX11Info::appTime(). It might be that QX11Info::appUserTime() is a slight
> "more" correct value.

Isn't that storing time and not serial? We need the 'serial' timestamp I believe.

[Kenneth Rohde Christiansen]

Simon, do we intent to fix this for QtWebKit2.0 or should we move to to 2.1?

[Kenneth Rohde Christiansen]

using the static QX11Info::appUserTime() (and 0/CurrentTime for that matter) did not fix the bug that context menus are not shown for windowless plugins on Qt. On the other hand we found around 5 other bugs, which Antonio is filing.

Simon, I think we should still integrate a patch using QX11Info::appUserTime() to avoid race conditions etc, as I think we have other bugs making the context menus now work. What is your take on that?

[Evan Martin]

Can you CC me on the other bugs?

How did you plumb this timing info through the WebKit event structure?  Or do you just call QX11Info::appUserTime() on the plugin side?

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> Can you CC me on the other bugs?
> 
> How did you plumb this timing info through the WebKit event structure?  Or do you just call QX11Info::appUserTime() on the plugin side?

The other patches were due to a newly introduced regression that we have how fixed.

Yes, if I understood Simon right we don't need to plumb the timing info though event structure (though I could easily do that). Still that doesn't seem to be the issue as it also didn't work with CurrentTime

[Kenneth Rohde Christiansen]

> Yes, if I understood Simon right we don't need to plumb the timing info though event structure (though I could easily do that). Still that doesn't seem to be the issue as it also didn't work with CurrentTime

Propagating the timestamps is apparently harder than I thought, as we receive DOM events (MouseEvent etc) and not Platform*Event. It should be possible to add a setX11TimeStamp() to Event.h which defaults to 0 (CurrentTime), but I don't know if that is worth it or useful for Chromium as well.

[Evan Martin]

Yes, it'd be nice.  In Chromium's case, due to our process split, it's actually more important.

Basically we do:
  X event -> browser process -> convert to PlatformMouseEvent
then we IPC that to the renderer process, which does
  PlatformMouseEvent -> WebKit -> WebKit's plugin code -> message to plugin
then we IPC that to the plugin process, which does
  plugin event -> plugin -> X

And we need the the time that comes out in that last step to match the X event time in the first step.

[Kenneth Rohde Christiansen]

Created attachment 56024 [details]
Diff

There is not really obvious beautiful way to pass this through to the plugin, but here is atleast a try. Comments are ofcourse welcome.

[Evan Martin]

Comment on attachment 56024 [details]
Diff

This looks pretty good to me.  I noticed in some places you call it "platformTime" and in others "platformTimeStamp", probably would be good to unify on one.

Where is PlatformTimeStamp defined?  It needs to be at least a "long" for X time, I think.

[Kenneth Rohde Christiansen]

(In reply to comment #18)
> (From update of attachment 56024 [details])
> This looks pretty good to me.  I noticed in some places you call it "platformTime" and in others "platformTimeStamp", probably would be good to unify on one.

Yes, that can be changed, the other timestamp uses a variable called m_createTime, so I named it m_platformTime for consistance with this variable. No biggie though.

> Where is PlatformTimeStamp defined?  It needs to be at least a "long" for X time, I think.

That was defined in PlatformEvent.h which I forgot adding to the diff at first, but then remembered and then uploaded the first diff :-) my bad!

It had the following define

+#if OS(LINUX)
+typedef unsigned long PlatformTimeStamp;
+#else
+typedef unsigned long long PlatformTimeStamp;
+#endif

[Brett Wilson (Google)]

I wired this into Chrome and found that it did not fix the plugin issues we were having. I'm going to try to track down this problem first and we'll see if this is really necessary. Kenneth: does this fix anything specific on your end?

[Kenneth Rohde Christiansen]

It only fixes part of our problems, and I haven't had time to dig any further. Thanks for looking into this.

[Brett Wilson (Google)]

Created attachment 57385 [details]
Patch with PlatformTime.h added and Chromium changes

This adds the missing PlatformTime.h file from the previous patch, and includes the changes necessary to implement this in the Chromium embedding layer on GTK. It's complete as far as I know, but doesn't seem to solve the problem.

[David Benjamin]

Created attachment 111123 [details]
Updated patch to track X11 timestamps

I've reworked the patch so that it applies to a more recent WebKit, and put the right timestamps into PluginViewGtk while I was at it. (Note: this has only been tested on Chromium.) The remaining pieces needed to take care of Chromium's problems are detailed in the comment below. For Flash itself, having a correct timestamp ends up being of only tangential benefit. As far as I can tell, Flash ignores that value, though it really shouldn't be. (See footnote 1 in linked comment.)

http://code.google.com/p/chromium/issues/detail?id=40157#c27

[WebKit Review Bot]

Attachment 111123 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   3e87a7a..6b4f546  master     -> origin/master
	M	Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp
	M	Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp
	M	Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h
	M	Source/WebKit2/ChangeLog
r97624 = 72bc733e2228c4c4700184f4f2605cd116ca259c (refs/remotes/trunk)
	M	LayoutTests/http/tests/css/link-css-disabled-value-with-slow-loading-sheet-in-error.html
	M	LayoutTests/ChangeLog
r97625 = 15b67984c99101032187a39ea4b6128e043611e3 (refs/remotes/trunk)
	M	Tools/ChangeLog
r97626 = 6b4f546b3f02d11f353a2c351bbab4d0b614aee1 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Support for plugins was removed in https://trac.webkit.org/changeset/265753/webkit.

[Radar WebKit Bug Importer]

<rdar://problem/96050260>