Bug 194367 – Missing media controls when WebKit is built with Python3

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Michael Catanzaro]

This video plays perfectly fine in WebKitGTK+ 2.23.3:

https://ftp.osuosl.org/pub/fosdem/2019/H.1309/media_gstreamer_wpe.webm

Except the media controls are missing! It should surely be displaying WebKit's media controls.

[Philippe Normand]

I can't reproduce this issue with MiniBrowser from ToT.

[Michael Catanzaro]

Indeed, it works in MiniBrowser, but it's broken in the GNOME runtime. When 2.23.90 is released we should check if it's fixed there.

[Philippe Normand]

(In reply to Michael Catanzaro from comment #2)
> Indeed, it works in MiniBrowser, but it's broken in the GNOME runtime. When
> 2.23.90 is released we should check if it's fixed there.

Ping :)

[Michael Catanzaro]

Nope, still no media controls with 2.24.0.

[Philippe Normand]

Looks like an Ephy bug then. Before reopening please provide a reproducer with MiniBrowser.

[Michael Catanzaro]

(In reply to Philippe Normand from comment #5)
> Looks like an Ephy bug then. Before reopening please provide a reproducer
> with MiniBrowser.

You know Epiphany doesn't have anything to do with media controls.

The WebKit development jhbuild is old and stale, whereas the GNOME master runtime is shiny and new. No wonder the set of bugs is different.

[Michael Catanzaro]

I checked Fedora 30's Epiphany, with WebKitGTK 2.23.91. The media controls slider is working fine. This strongly suggests that it's not Epiphany's fault, but something else in the runtime. You should reopen this until it's fixed.

[Philippe Normand]

(In reply to Michael Catanzaro from comment #6)
> (In reply to Philippe Normand from comment #5)
> > Looks like an Ephy bug then. Before reopening please provide a reproducer
> > with MiniBrowser.
> 
> You know Epiphany doesn't have anything to do with media controls.
> 

Of course I know that.
But perhaps Ephy uses a different API to load URIs?

> The WebKit development jhbuild is old and stale, whereas the GNOME master
> runtime is shiny and new. No wonder the set of bugs is different.

I've just updated my local WebKit-Flatpak to the 3.32 GNOME Platform/Sdk and wasn't able to reproduce the bug there either (with MiniBrowser).

[Michael Catanzaro]

Try the master SDK, that's what Ephy Tech Preview uses. It probably has newer GStreamer or something.

[Michael Catanzaro]

(In reply to Philippe Normand from comment #8)
> Of course I know that.
> But perhaps Ephy uses a different API to load URIs?

webkit_web_view_load_uri()?

[Xabier Rodríguez Calvar]

(In reply to Michael Catanzaro from comment #9)
> Try the master SDK, that's what Ephy Tech Preview uses. It probably has
> newer GStreamer or something.

Controls are not a GStreamer thing.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7)
> I checked Fedora 30's Epiphany, with WebKitGTK 2.23.91. The media controls
> slider is working fine. This strongly suggests that it's not Epiphany's
> fault, but something else in the runtime. You should reopen this until it's
> fixed.

OK, now one month later with WebKitGTK 2.24.0 it's broken. So I don't think WebKit version is to blame here. Whatever is to blame must have made it into Fedora within the past month.

There are errors in the web inspector when playing Phil's video in the first comment:

[Error] SyntaxError: Unexpected string literal 'v'

[Error] Failed to load resource: Plug-in handled load (media_gstreamer_wpe.webm, line 0)

(Plus an unrelated favicon error.) I think these errors are our hints.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #12)
> [Error] Failed to load resource: Plug-in handled load
> (media_gstreamer_wpe.webm, line 0)

This is coming from WebFrameLoaderClient::committedLoad and it seems to be intentional for any standalone media document, which is weird because of course intentional internal errors shouldn't reach the web inspector.

[Michael Catanzaro]

This one is really bad.

[Guilaume Ayoub]

Is there a way to help fixing this issue?

[Philippe Normand]

The issue is that, for some reason, the shadow DOM (supposed to contain the media controls) of the <video> element is empty. You can check that with the web-inspector.

But since this issue happens only with the Ephy flatpak runtime, it's quite hard to track down...

And it's not only for MediaDocuments, it's for all <video> relying on WebKit's media controls.

[Guilaume Ayoub]

(In reply to Philippe Normand from comment #16)
> The issue is that, for some reason, the shadow DOM (supposed to contain the
> media controls) of the <video> element is empty. You can check that with the
> web-inspector.
> 
> But since this issue happens only with the Ephy flatpak runtime, it's quite
> hard to track down...
> 
> And it's not only for MediaDocuments, it's for all <video> relying on
> WebKit's media controls.

And it's the same for <audio> too.

I've reinstalled older versions of Epiphany (the oldest being 3.24.0) with latest WebKitGTK+, and the controls are always missing. I think that controls were visible at least 2 years ago, so I can imagine that the bug is not caused by a change in Epiphany.

Bisecting WebKitGTK+ is really painful, but I can do this. If anyone knows where "SyntaxError: Unexpected string literal 'v'" comes from (and if it's related), that would be helpful!

[Philippe Normand]

Seems like that error comes from Source/JavaScriptCore/parser/Parser.cpp but I doubt it's related.

[Philippe Normand]

With Ephy 3.32.1.2 (wkgtk 2.24.3) from Debian Testing, I do see controls.

[Michael Catanzaro]

(In reply to Guilaume Ayoub from comment #17) 
> Bisecting WebKitGTK+ is really painful, but I can do this.

Thanks so much. But I'm really not sure a WebKit change is to blame. You might need to do a flatpak-bisect to bisect the runtime itself, except this bug is now so old that there's probably not enough old runtimes left on the server, so it's probably impossible.

[Michael Catanzaro]

Ah sorry, I guess you're testing with Fedora packages. Anyway, first step before we start bisecting is to pin down which package change caused the problem. If we can verify that it's really caused by a WebKit change, then it's bisectable, but unfortunately I'm not confident that's what we'll find. :/

[Guilaume Ayoub]

(In reply to Michael Catanzaro from comment #21)
> Ah sorry, I guess you're testing with Fedora packages.

I'm not testing with Fedora, I'm using Gentoo. Fedora's not to blame here :).

> Anyway, first step
> before we start bisecting is to pin down which package change caused the
> problem. If we can verify that it's really caused by a WebKit change, then
> it's bisectable, but unfortunately I'm not confident that's what we'll find.
> :/

I can at least rebuild an old version of WebKit and see if the controls are visible. That way we can be sure that the problem is not caused by another package.

[Michael Catanzaro]

OK, thanks.

Figuring out what package would be tremendously helpful.

[Guilaume Ayoub]

Media controls are visible with Epiphany 2.28.0 + WebKitGTK 2.20.3.

I continue the bisection, but it's long because Ephy and WebKitGTK versions can't be mixed as we want…

[Guilaume Ayoub]

Media controls are visible with Epiphany 2.30.3 + WebKitGTK 2.22.3.

[Michael Catanzaro]

You mean Epiphany 3.28.0 and Epiphany 3.30.3?

There's no need to build Epiphany at all because this bug occurs in MiniBrowser. On Fedora it's packaged as /usr/libexec/webkit2gtk-4.0/MiniBrowser. Probably on Gentoo it would be under /usr/lib/.

[Guilaume Ayoub]

Media controls are also visible with Epiphany 2.31.91 + WebKitGTK 2.23.2.

As the original bug report is for WebKitGTK 2.23.3, I'll compile this version and check that the media controls are missing. We'll be sure that the problem comes from a change between 2.23.2 and 2.23.3.

It's time for me to sleep while my computer is compiling, see you tomorrow!

(In reply to Michael Catanzaro from comment #26)
> You mean Epiphany 3.28.0 and Epiphany 3.30.3?
> 
> There's no need to build Epiphany at all because this bug occurs in
> MiniBrowser. On Fedora it's packaged as
> /usr/libexec/webkit2gtk-4.0/MiniBrowser. Probably on Gentoo it would be
> under /usr/lib/.

Oh, as it was said in Comment #2 that the bug wasn't visible in MiniBrowser, I assumed that I had to compile Epiphany too. Well, compiling Epiphany is so fast that it doesn't bother me…

[Carlos Alberto Lopez Perez]

I think this issue may be the same (or very similar) than bug 200894

Can you check in your CMakeCache.txt file for the build the value you have configured for PYTHON_EXECUTABLE? If its pointing to python3 please test to rebuild with python2. First install python2 (if needed) and then pass something like "-DPYTHON_EXECUTABLE=/usr/bin/python2.7". Test this from a clean build state.

[Guilaume Ayoub]

(In reply to Carlos Alberto Lopez Perez from comment #28)
> I think this issue may be the same (or very similar) than bug 200894
> 
> Can you check in your CMakeCache.txt file for the build the value you have
> configured for PYTHON_EXECUTABLE? If its pointing to python3 please test to
> rebuild with python2. First install python2 (if needed) and then pass
> something like "-DPYTHON_EXECUTABLE=/usr/bin/python2.7". Test this from a
> clean build state.

You're probably right. I have controls with 2.23.3 compiled with Python 2.7, but I didn't have them with 2.24.x versions compiled with Python 3.7.

Do you want some help finding the problem in Python scripts?

[Philippe Normand]

Carlos!!!!!
You nailed the bug!

[Philippe Normand]

Guillaume,

Maybe start with checking these 2 fellows?

Source/WebCore/css/makeSelectorPseudoClassAndCompatibilityElementMap.py
Source/WebCore/css/makeSelectorPseudoElementsMap.py

[Guilaume Ayoub]

(In reply to Philippe Normand from comment #31)
> Guillaume,
> 
> Maybe start with checking these 2 fellows?
> 
> Source/WebCore/css/makeSelectorPseudoClassAndCompatibilityElementMap.py
> Source/WebCore/css/makeSelectorPseudoElementsMap.py

They generate the same files with Py2 and Py3.

I've also tried Source/JavaScriptCore/Scripts/inline-and-minify-stylesheets-and-scripts.py, it gives the same results.

[Philippe Normand]

We would need the build to fail when the python3 raises an exception, basically. I can have a look at this later on today.

[Guilaume Ayoub]

(In reply to Philippe Normand from comment #33)
> We would need the build to fail when the python3 raises an exception,
> basically. I can have a look at this later on today.

I'm not sure that it raises an exception, it may just generate a JS or CSS file with a syntax error.

[Philippe Normand]

Comparing 2 builds, Source/JavaScriptCore/Scripts/make-js-file-arrays.py generates broken UserAgentScriptsData.cpp for Python3 builds.

[Philippe Normand]

Starting a patch, the js minifier is broken as well...

[Philippe Normand]

Created attachment 376763 [details]
Patch

[Philippe Normand]

Fixes #200894 as well \o/

[Guilaume Ayoub]

(In reply to Philippe Normand from comment #37)
> Created attachment 376763 [details]
> Patch

It works perfectly, thanks a lot everybody! <3

[Carlos Alberto Lopez Perez]

Comment on attachment 376763 [details]
Patch

It seems to work, but I'm a bit puzzled why it still gives different results for python2 and python3.

See:
$ python2 Source/JavaScriptCore/Scripts/make-js-file-arrays.py test2.h test2.cpp -n WebCore Source/WebCore/Modules/mediacontrols/mediaControlsBase.js
$ python3 Source/JavaScriptCore/Scripts/make-js-file-arrays.py test3.h test3.cpp -n WebCore Source/WebCore/Modules/mediacontrols/mediaControlsBase.js
$ diff -u test2.h test3.h
--- test2.h	2019-08-20 13:02:55.818755324 +0000
+++ test3.h	2019-08-20 13:03:06.312126092 +0000
@@ -1,3 +1,3 @@
 namespace WebCore {
-extern const char mediaControlsBaseJavaScript[35332];
+extern const char mediaControlsBaseJavaScript[46873];
 } // namespace WebCore

$ diff -u test2.cpp test3.cpp
[... redacted, lots of diffs ... ]

Any idea what causes this?

[Carlos Alberto Lopez Perez]

*** Bug 200894 has been marked as a duplicate of this bug. ***

[Philippe Normand]

The biggest issue seems to be in the minifier, the bug was introduced during its port to Python3 I think:

> /home/phil/WebKit/Source/JavaScriptCore/Scripts/jsmin.py(89)write()
-> self.outs.write(str(char))
(Pdb) char
b'v'
(Pdb) str(char)
"b'v'"
(Pdb) 

the write function declared line 79 doesn't handle byte values, you can see the b is embedded in the string. This is where the SyntaxError comes from at runtime.

[Philippe Normand]

Though that doesn't explain why the array sizes differ... It's a bit odd :)

[Philippe Normand]

(In reply to Philippe Normand from comment #43)
> Though that doesn't explain why the array sizes differ... It's a bit odd :)

Ah found it. In that same write function, for Python3 the return_buf should be a byte string, and the 'return' constant as well.

[Philippe Normand]

Comment on attachment 376763 [details]
Patch

This needs some more investigation...

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #45)
> Comment on attachment 376763 [details]
> Patch
> 
> This needs some more investigation...

By printing the generated char array to stdout with a small C program I can see that in the case of python3 the JS is not minified.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #46)
> (In reply to Philippe Normand from comment #45)
> > Comment on attachment 376763 [details]
> > Patch
> > 
> > This needs some more investigation...
> 
> By printing the generated char array to stdout with a small C program I can
> see that in the case of python3 the JS is not minified.

The issue is caused because all over the function JavascriptMinify:minify() the code does comparisons with strings, which fail on python3 because now the type for the variable is bytes

b'/' == '/' is False

Something like this is needed:

--- a/Source/JavaScriptCore/Scripts/jsmin.py
+++ b/Source/JavaScriptCore/Scripts/jsmin.py
@@ -82,11 +82,17 @@ class JavascriptMinify(object):
             if str(char) in 'return':
                 self.return_buf += char
                 self.is_return = self.return_buf == 'return'
+            if is_3:
+                char=str.encode(char)
             self.outs.write(char)
             if self.is_return:
                 self.return_buf = ''
 
-        read = self.ins.read
+        def read(char):
+            if is_3:
+                return self.ins.read(char).decode()
+            else:
+                return self.ins.read(char)


Maybe there is a better way of doing this that doesn't require converting between str and bytes back and fort?

[Philippe Normand]

Ah I missed the read part earlier.

I think the simplest is likely to convert byte strings to utf-8 in Python3, so that the Python2 code can work as-is?

[Philippe Normand]

I may have an updated patch but it needs some more testing wrt l10n files.

[Philippe Normand]

Created attachment 376851 [details]
WiP patch

This is my current status. However I would like to move this jsmin thing to webkitpy and add unit-tests. But right now I don't have time for this, so unless someone else picks up the task I will try to get back to it soon.

[Carlos Alberto Lopez Perez]

(In reply to Philippe Normand from comment #50)
> Created attachment 376851 [details]
> WiP patch
> 
> This is my current status. However I would like to move this jsmin thing to
> webkitpy and add unit-tests. But right now I don't have time for this, so
> unless someone else picks up the task I will try to get back to it soon.

I don't think moving this to webkitpy is a good idea. This needs to be shipped on release tarballs as is part of the built, and release tarballs don't include webkitpy (AFAIK).

On the other hand the unit tests for webkitpy are currently ran only with python2, we don't have any facility to run unit tests with python3.

Maybe an idea is to test this from webkitpy (without moving it, simply call it where is it from webkitpy) and also start testing webkitpy with python3 (on top of python2). But this is not going to be easy and is already out of the scope of this bug. I suggest to simply fix the issue here and after that (if you want), on a new bug, start working on this extra tests with python3.

[Adrian Perez]

Comment on attachment 376851 [details]
WiP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=376851&action=review

Overall patch idea LGTM, but I think there is one situation
which is not being covered — please check the comment.

I agree with Carlos López that it would be good to try
things with Python 3.x, but that is indeed scope for a
different bug.

> Source/JavaScriptCore/Scripts/jsmin.py:56
> +    ins = klass(js.readall())

This is problematic, because from the code above which checks whether “js”
is an “unicode” instance, it looks to me that it is acceptable to pass a
string to “jsmin()” — I think something like the following would be better:

  text_class = str if is_3 else unicode
  bytes_class = bytes if is_3 else str
  if hasattr(js, "readall"):
      js = js.readall()
  elif isinstance(js, text_class):
      js = js.encode("utf-8")

  # In the end we always want a sequence of bytes.
  assert isinstance(js, bytes_class)  
  ins = klass(js)
  # ...

I think something in the spirit of the example above should cover all bases.

[Adrian Perez]

FWIW, shouldn't we tag this with [WPE] as well? I was under
the impression that WPE uses media controls with CSS/JS.

[Philippe Normand]

Actually I suppose that the Apple ports are also affected by this issue.

[Adrian Perez]

(In reply to Philippe Normand from comment #54)
> Actually I suppose that the Apple ports are also affected by this issue.

Yes, that will be the case when they start using Python 3.x for
building later on; IIRC for the moment they are still using 2.x

[Carlos Alberto Lopez Perez]

I think I have a patch that fixes this :)

I tested to run manually the command make-js-file-arrays.py with different source files and compared the outputs for python2 and python3. With this patch it is generating always the same result as far as I was able to test. I also tested to match with the output it was generating before this patch for python2 and it matches.

I also compared the generated sources after a build before and after this patch, and with python2 and python3, and the sources affected by this seem to match exactly as expected.

And of course this bug and bug 200894 are fixed with this.

I'll upload it now for review/test.

[Carlos Alberto Lopez Perez]

Created attachment 377080 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 377080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377080&action=review

> Source/JavaScriptCore/Scripts/jsmin.py:94
> +                raise ValueError("ERROR: The script jsmin.py only can handle text input, but it received input with type %s" % type(char))

I think it should be "can only handle" and "received input of type"

[Carlos Alberto Lopez Perez]

Committed r249095: <https://trac.webkit.org/changeset/249095>

[Radar WebKit Bug Importer]

<rdar://problem/54706396>