New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial QtWebEngine refactoring #1629

Merged
merged 134 commits into from Jul 11, 2016

Conversation

Projects
None yet
7 participants
@The-Compiler
Collaborator

The-Compiler commented Jul 7, 2016

This is the initial refactoring trying to get (almost) all QtWebKit access to happen via a well-defined API.

Some things are still "cheating" by accessing _widget; I'll look at those later. The goal of this is to get QtWebKit to still work as before, with as much as possible moved under the new API.

I'd really appreciate some review for this, which is why I decided to open a PR - @Kingdread @rcorre @antoyo @lamarpavel @lahwaacz anyone feels like taking a look?

The most important thing is probably the API design in tab.py. This will also be part of the plugin API once there is such a thing.

Things I plan to do (probably tomorrow) before merging this:

  • Rename some files (tab.py turns out to be a bad name, better suggestions?) and classes (WebView -> WebKit)
  • Implement stubs which simply do nothing but log an error for all QtWebEngine methods, so people can start experimenting without crashes


This change is Reviewable

The-Compiler added some commits Jun 13, 2016

Make self._widget private in wrappers
While we need to set it from the outside (from AbstractTab) this still
is not considered public API for the rest of the code, so let's make it
private.
@Kingdread

This comment has been minimized.

Show comment
Hide comment
@Kingdread

Kingdread Jul 8, 2016

Collaborator

It's probably the lesser evil and fine, especially since most implementations are one-liners anyway. You could add another intermediate base class (like WebKitHistory <- QtHistoryBase <- AbstractHistory), but as you said, then you'd have three places to search the code, and it's probably not worth for those 7 lines.

I definitely wouldn't put it in the abstract class, in my opinion only implementations that are expressible via other abstract methods belong there.

Collaborator

Kingdread commented Jul 8, 2016

It's probably the lesser evil and fine, especially since most implementations are one-liners anyway. You could add another intermediate base class (like WebKitHistory <- QtHistoryBase <- AbstractHistory), but as you said, then you'd have three places to search the code, and it's probably not worth for those 7 lines.

I definitely wouldn't put it in the abstract class, in my opinion only implementations that are expressible via other abstract methods belong there.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 10, 2016

Collaborator

I pushed some more commits - some changes are unrelated to QtWebEngine, but I just noticed a lot of things and decided to fix them 😆

Now only the rename is left, and then I'll merge this.

Collaborator

The-Compiler commented Jul 10, 2016

I pushed some more commits - some changes are unrelated to QtWebEngine, but I just noticed a lot of things and decided to fix them 😆

Now only the rename is left, and then I'll merge this.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 10, 2016

Collaborator

Things are looking good locally, but suddenly there are segfaults in test_config on Travis for older Qt versions?!

Collaborator

The-Compiler commented Jul 10, 2016

Things are looking good locally, but suddenly there are segfaults in test_config on Travis for older Qt versions?!

@fermuch

This comment has been minimized.

Show comment
Hide comment
@fermuch

fermuch Jul 10, 2016

I'm testing this branch with real usage. So far, so good.

with master I had a crash on twitch.tv related to QtWebkit, but it seems gone.

fermuch commented Jul 10, 2016

I'm testing this branch with real usage. So far, so good.

with master I had a crash on twitch.tv related to QtWebkit, but it seems gone.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 10, 2016

Collaborator

@fermuch With --backend webengine? Otherwise you'll still be using QtWebKit 😉

Collaborator

The-Compiler commented Jul 10, 2016

@fermuch With --backend webengine? Otherwise you'll still be using QtWebKit 😉

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 10, 2016

Collaborator

The segfault happens here:

#0  0x00007ffff3e8fd92 in PyQtSlot::invoke (this=0x3409310, qargs=qargs@entry=0x7fffffff7850, self=0x80000005, self@entry=0x0, result=result@entry=0x0, no_receiver_check=<optimized out>)
    at ../../qpy/QtCore/qpycore_pyqtslot.cpp:111
#1  0x00007ffff3e8fe6d in PyQtSlot::invoke (this=<optimized out>, qargs=qargs@entry=0x7fffffff7850, no_receiver_check=<optimized out>) at ../../qpy/QtCore/qpycore_pyqtslot.cpp:76
#2  0x00007ffff3e9b730 in PyQtSlotProxy::unislot (this=0x2b27e70, qargs=0x7fffffff7850) at ../../qpy/QtCore/qpycore_pyqtslotproxy.cpp:205
#3  0x00007ffff3e9c077 in PyQtSlotProxy::qt_metacall (this=0x2b27e70, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffffff7850) at ../../qpy/QtCore/qpycore_pyqtslotproxy.cpp:170
#4  0x00007ffff38870a4 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007ffff38874ef in QObject::destroyed(QObject*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007fffec3332b8 in QWidget::~QWidget() () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#7  0x00007fffeb524a19 in sipQWidget::~sipQWidget (this=0x351e0b0, __in_chrg=<optimized out>) at sipQtWidgetspart9.cpp:30765
#8  0x00007ffff08c9b06 in ?? () from /root/outside/.tox/py34/lib/python3.4/site-packages/sip.cpython-34m-x86_64-linux-gnu.so
#9  0x00007ffff08cae29 in ?? () from /root/outside/.tox/py34/lib/python3.4/site-packages/sip.cpython-34m-x86_64-linux-gnu.so
#10 0x00000000005154c2 in ?? ()
#11 0x00000000004e884f in ?? ()
#12 0x00000000004e866b in ?? ()
#13 0x00000000005a4467 in ?? ()
#14 0x00000000004ddde8 in ?? ()
#15 0x00000000004dd50c in ?? ()
#16 0x00000000004cce5c in _PyObject_GC_Malloc ()

Bisecting wasn't very successful so far:

There are only 'skip'ped commits left to test.
The first bad commit could be any of:
b07cca023bcebc8783b982119fb3b9db6f4b5e9f
2136d00aa2dbaea0b23ee98b6f23d9bf74c3de6f
7859df74c4d6eab4f45ab7d2957e7aecd04ee2be
52e14950f193ed9444e0c581616d0405d74ab15f
949172809da76770079fbd27eb3201f80945c519
e36123735b7fc6b2304a4c6d9d34010306ce1a8c
fd8e66136ffc4eff414ff78552370717353b8d2b
0ab601aaf36288714e912d9c47f7f31cdfd32a07
34e39bed4e1f3b6c1300a0e23caff93ee321aa1d
d2ece6b5421d75cccf24a13bd0daa866a57fcc15
We cannot bisect more!

edit: Bisected to e361237... not sure what's triggering this yet.

Collaborator

The-Compiler commented Jul 10, 2016

The segfault happens here:

#0  0x00007ffff3e8fd92 in PyQtSlot::invoke (this=0x3409310, qargs=qargs@entry=0x7fffffff7850, self=0x80000005, self@entry=0x0, result=result@entry=0x0, no_receiver_check=<optimized out>)
    at ../../qpy/QtCore/qpycore_pyqtslot.cpp:111
#1  0x00007ffff3e8fe6d in PyQtSlot::invoke (this=<optimized out>, qargs=qargs@entry=0x7fffffff7850, no_receiver_check=<optimized out>) at ../../qpy/QtCore/qpycore_pyqtslot.cpp:76
#2  0x00007ffff3e9b730 in PyQtSlotProxy::unislot (this=0x2b27e70, qargs=0x7fffffff7850) at ../../qpy/QtCore/qpycore_pyqtslotproxy.cpp:205
#3  0x00007ffff3e9c077 in PyQtSlotProxy::qt_metacall (this=0x2b27e70, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffffff7850) at ../../qpy/QtCore/qpycore_pyqtslotproxy.cpp:170
#4  0x00007ffff38870a4 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#5  0x00007ffff38874ef in QObject::destroyed(QObject*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#6  0x00007fffec3332b8 in QWidget::~QWidget() () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#7  0x00007fffeb524a19 in sipQWidget::~sipQWidget (this=0x351e0b0, __in_chrg=<optimized out>) at sipQtWidgetspart9.cpp:30765
#8  0x00007ffff08c9b06 in ?? () from /root/outside/.tox/py34/lib/python3.4/site-packages/sip.cpython-34m-x86_64-linux-gnu.so
#9  0x00007ffff08cae29 in ?? () from /root/outside/.tox/py34/lib/python3.4/site-packages/sip.cpython-34m-x86_64-linux-gnu.so
#10 0x00000000005154c2 in ?? ()
#11 0x00000000004e884f in ?? ()
#12 0x00000000004e866b in ?? ()
#13 0x00000000005a4467 in ?? ()
#14 0x00000000004ddde8 in ?? ()
#15 0x00000000004dd50c in ?? ()
#16 0x00000000004cce5c in _PyObject_GC_Malloc ()

Bisecting wasn't very successful so far:

There are only 'skip'ped commits left to test.
The first bad commit could be any of:
b07cca023bcebc8783b982119fb3b9db6f4b5e9f
2136d00aa2dbaea0b23ee98b6f23d9bf74c3de6f
7859df74c4d6eab4f45ab7d2957e7aecd04ee2be
52e14950f193ed9444e0c581616d0405d74ab15f
949172809da76770079fbd27eb3201f80945c519
e36123735b7fc6b2304a4c6d9d34010306ce1a8c
fd8e66136ffc4eff414ff78552370717353b8d2b
0ab601aaf36288714e912d9c47f7f31cdfd32a07
34e39bed4e1f3b6c1300a0e23caff93ee321aa1d
d2ece6b5421d75cccf24a13bd0daa866a57fcc15
We cannot bisect more!

edit: Bisected to e361237... not sure what's triggering this yet.

@fermuch

This comment has been minimized.

Show comment
Hide comment
@fermuch

fermuch Jul 10, 2016

@fermuch With --backend webengine? Otherwise you'll still be using QtWebKit 😉

Hehe. Guess I was trying with QtWebKit.
I'll try again in a few min.

fermuch commented Jul 10, 2016

@fermuch With --backend webengine? Otherwise you'll still be using QtWebKit 😉

Hehe. Guess I was trying with QtWebKit.
I'll try again in a few min.

@rcorre

This comment has been minimized.

Show comment
Hide comment
@rcorre

rcorre Jul 10, 2016

Collaborator

I get a segfault after closing with the webengine backend (but not webkit).

13:59:01 DEBUG    destroy    app:exit:774 Now calling QApplication::exit.
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: 0
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: main-window
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: message-bridge
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: download-manager
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: statusbar
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: prompt
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: tabbed-browser
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: completion
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: completer
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: mode-manager
./run.sh: line 2:  1249 Segmentation fault      (core dumped) .venv/bin/python3 -m qutebrowser --temp-basedir "$@"

I cut out a bunch of Focus object changed lines from the above log.

Collaborator

rcorre commented Jul 10, 2016

I get a segfault after closing with the webengine backend (but not webkit).

13:59:01 DEBUG    destroy    app:exit:774 Now calling QApplication::exit.
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: 0
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: main-window
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: message-bridge
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: download-manager
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: statusbar
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: prompt
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: tabbed-browser
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: completion
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: completer
13:59:01 DEBUG    destroy    objreg:on_destroyed:118 schedule removal: mode-manager
./run.sh: line 2:  1249 Segmentation fault      (core dumped) .venv/bin/python3 -m qutebrowser --temp-basedir "$@"

I cut out a bunch of Focus object changed lines from the above log.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 10, 2016

Collaborator

Yeah, I plan to investigate segfaults on closing somewhen later, those are always really tricky to work around...

Collaborator

The-Compiler commented Jul 10, 2016

Yeah, I plan to investigate segfaults on closing somewhen later, those are always really tricky to work around...

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 10, 2016

Collaborator

With the segfault from #1629 (comment), seems like it segfaults when trying to run the QObject::destroyed slot which is connected here:

[58]   /home/florian/proj/qutebrowser/git/tests/unit/completion/test_models.py(310)test_tab_completion()
-> fake_web_tab(QUrl('https://github.com'), 'GitHub', 0),
[59]   /home/florian/proj/qutebrowser/git/tests/helpers/stubs.py(255)__init__()
-> super().__init__(win_id=0)
[60]   /home/florian/proj/qutebrowser/git/qutebrowser/browser/browsertab.py(457)__init__()
-> tab_registry[self.tab_id] = self
[61]   /home/florian/proj/qutebrowser/git/qutebrowser/utils/objreg.py(85)__setitem__()
-> obj.destroyed.connect(func)

I'll just skip the affected tests with older PyQt versions...

Collaborator

The-Compiler commented Jul 10, 2016

With the segfault from #1629 (comment), seems like it segfaults when trying to run the QObject::destroyed slot which is connected here:

[58]   /home/florian/proj/qutebrowser/git/tests/unit/completion/test_models.py(310)test_tab_completion()
-> fake_web_tab(QUrl('https://github.com'), 'GitHub', 0),
[59]   /home/florian/proj/qutebrowser/git/tests/helpers/stubs.py(255)__init__()
-> super().__init__(win_id=0)
[60]   /home/florian/proj/qutebrowser/git/qutebrowser/browser/browsertab.py(457)__init__()
-> tab_registry[self.tab_id] = self
[61]   /home/florian/proj/qutebrowser/git/qutebrowser/utils/objreg.py(85)__setitem__()
-> obj.destroyed.connect(func)

I'll just skip the affected tests with older PyQt versions...

Skip tests using fake_web_tab on PyQt < 5.6
For some weird reason they cause a segfault in QObject::disconnect since
fake_web_tab was converted to a fixture...

See #1638
@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 10, 2016

Collaborator

I opened #1638 now, and skipped all related tests (around 15) on PyQt < 5.6... Clearly not ideal, but I definitely won't be debugging this today.

Collaborator

The-Compiler commented Jul 10, 2016

I opened #1638 now, and skipped all related tests (around 15) on PyQt < 5.6... Clearly not ideal, but I definitely won't be debugging this today.

@fermuch

This comment has been minimized.

Show comment
Hide comment
@fermuch

fermuch Jul 10, 2016

This might be an issue on my end, but i'm not sure.

Traceback (most recent call last):
  File "/usr/lib64/python3.5/site-packages/qutebrowser/app.py", line 198, in _process_args
    _open_startpage()
  File "/usr/lib64/python3.5/site-packages/qutebrowser/app.py", line 309, in _open_startpage
    tabbed_browser.tabopen(url)
  File "/usr/lib64/python3.5/site-packages/qutebrowser/mainwindow/tabbedbrowser.py", line 382, in tabopen
    tab = browsertab.create(win_id=self._win_id, parent=self)
  File "/usr/lib64/python3.5/site-packages/qutebrowser/browser/browsertab.py", line 47, in create
    from qutebrowser.browser.webengine import webenginetab
  File "/usr/lib64/python3.5/site-packages/qutebrowser/browser/webengine/webenginetab.py", line 29, in <module>
    from PyQt5.QtWebEngineWidgets import QWebEnginePage
ImportError: QtWebEngineWidgets must be imported before a QCoreApplication instance is created

I just recompiled PyQt5 5.6.1_pre1604271126 (latest on Gentoo) with support for QtWebEngine and i'm getting that error while trying to start the app with the webengine backend.

fermuch commented Jul 10, 2016

This might be an issue on my end, but i'm not sure.

Traceback (most recent call last):
  File "/usr/lib64/python3.5/site-packages/qutebrowser/app.py", line 198, in _process_args
    _open_startpage()
  File "/usr/lib64/python3.5/site-packages/qutebrowser/app.py", line 309, in _open_startpage
    tabbed_browser.tabopen(url)
  File "/usr/lib64/python3.5/site-packages/qutebrowser/mainwindow/tabbedbrowser.py", line 382, in tabopen
    tab = browsertab.create(win_id=self._win_id, parent=self)
  File "/usr/lib64/python3.5/site-packages/qutebrowser/browser/browsertab.py", line 47, in create
    from qutebrowser.browser.webengine import webenginetab
  File "/usr/lib64/python3.5/site-packages/qutebrowser/browser/webengine/webenginetab.py", line 29, in <module>
    from PyQt5.QtWebEngineWidgets import QWebEnginePage
ImportError: QtWebEngineWidgets must be imported before a QCoreApplication instance is created

I just recompiled PyQt5 5.6.1_pre1604271126 (latest on Gentoo) with support for QtWebEngine and i'm getting that error while trying to start the app with the webengine backend.

@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 11, 2016

Collaborator

@fermuch I don't know why I don't get that error, but can you check if b4f993e helps?

Collaborator

The-Compiler commented Jul 11, 2016

@fermuch I don't know why I don't get that error, but can you check if b4f993e helps?

@The-Compiler The-Compiler merged commit b4f993e into master Jul 11, 2016

4 of 6 checks passed

codecov/patch 74.53% of diff hit (target 82.78%)
Details
codecov/project 81.85% (-0.94%) compared to 3bb5b5d
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@The-Compiler

This comment has been minimized.

Show comment
Hide comment
@The-Compiler

The-Compiler Jul 11, 2016

Collaborator

I merged this now - @fermuch if you still have issues with the newest code, can you please open a new issue?

Collaborator

The-Compiler commented Jul 11, 2016

I merged this now - @fermuch if you still have issues with the newest code, can you please open a new issue?

@The-Compiler The-Compiler deleted the qtwebengine branch Jul 11, 2016

@The-Compiler The-Compiler restored the qtwebengine branch Jul 11, 2016

@The-Compiler The-Compiler deleted the qtwebengine branch Jul 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment