From 954773124a453107e4f4a79b8c3342924f34d703 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 28 Aug 2019 09:56:45 -0700 Subject: [PATCH 1/4] BUGZ-1363: crash when checking for web content restrictions --- interface/src/ui/InteractiveWindow.cpp | 19 +++++++++++++++---- libraries/ui/src/QmlWindowClass.cpp | 4 +--- .../ui/src/ui/types/ContextAwareProfile.cpp | 18 ++++++++++++------ .../ui/src/ui/types/ContextAwareProfile.h | 8 ++++---- libraries/ui/src/ui/types/FileTypeProfile.cpp | 4 ++-- .../ui/src/ui/types/HFWebEngineProfile.cpp | 2 +- libraries/ui/src/ui/types/RequestFilters.cpp | 6 +++--- libraries/ui/src/ui/types/RequestFilters.h | 4 ++-- 8 files changed, 40 insertions(+), 25 deletions(-) diff --git a/interface/src/ui/InteractiveWindow.cpp b/interface/src/ui/InteractiveWindow.cpp index 9145b12d30..af3562e747 100644 --- a/interface/src/ui/InteractiveWindow.cpp +++ b/interface/src/ui/InteractiveWindow.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -228,9 +229,15 @@ InteractiveWindow::InteractiveWindow(const QString& sourceUrl, const QVariantMap _dockWidget->setObjectName("DockedWidget"); mainWindow->addDockWidget(dockArea, _dockWidget.get()); } else { - auto offscreenUi = DependencyManager::get(); - // Build the event bridge and wrapper on the main thread - offscreenUi->loadInNewContext(CONTENT_WINDOW_QML, [&](QQmlContext* context, QObject* object) { + auto contextInitLambda = [&](QQmlContext* context) { +#if !defined(Q_OS_ANDROID) + // If the restricted flag is on, override the FileTypeProfile and HFWebEngineProfile objects in the + // QML surface root context with local ones + ContextAwareProfile::restrictContext(context, false); +#endif + }; + + auto objectInitLambda = [&](QQmlContext* context, QObject* object) { _qmlWindowProxy = std::shared_ptr(new QmlWindowProxy(object, nullptr), qmlWindowProxyDeleter); context->setContextProperty(EVENT_BRIDGE_PROPERTY, _interactiveWindowProxy.get()); if (properties.contains(ADDITIONAL_FLAGS_PROPERTY)) { @@ -286,7 +293,11 @@ InteractiveWindow::InteractiveWindow(const QString& sourceUrl, const QVariantMap } object->setObjectName("InteractiveWindow"); object->setProperty(SOURCE_PROPERTY, sourceURL); - }); + }; + auto offscreenUi = DependencyManager::get(); + + // Build the event bridge and wrapper on the main thread + offscreenUi->loadInNewContext(CONTENT_WINDOW_QML, objectInitLambda, contextInitLambda); } } diff --git a/libraries/ui/src/QmlWindowClass.cpp b/libraries/ui/src/QmlWindowClass.cpp index 1140dbb079..4690a720f2 100644 --- a/libraries/ui/src/QmlWindowClass.cpp +++ b/libraries/ui/src/QmlWindowClass.cpp @@ -136,10 +136,8 @@ void QmlWindowClass::initQml(QVariantMap properties) { #if !defined(Q_OS_ANDROID) // If the restricted flag is on, override the FileTypeProfile and HFWebEngineProfile objects in the // QML surface root context with local ones - qDebug() << "Context initialization lambda"; + ContextAwareProfile::restrictContext(context, _restricted); if (_restricted) { - qDebug() << "Restricting web content"; - ContextAwareProfile::restrictContext(context); FileTypeProfile::registerWithContext(context); HFWebEngineProfile::registerWithContext(context); } diff --git a/libraries/ui/src/ui/types/ContextAwareProfile.cpp b/libraries/ui/src/ui/types/ContextAwareProfile.cpp index 98cc94ec10..8e363e6546 100644 --- a/libraries/ui/src/ui/types/ContextAwareProfile.cpp +++ b/libraries/ui/src/ui/types/ContextAwareProfile.cpp @@ -10,6 +10,8 @@ // #include "ContextAwareProfile.h" +#include +#include #if !defined(Q_OS_ANDROID) @@ -17,16 +19,20 @@ static const QString RESTRICTED_FLAG_PROPERTY = "RestrictFileAccess"; -ContextAwareProfile::ContextAwareProfile(QQmlContext* parent) : - QQuickWebEngineProfile(parent), _context(parent) { } +ContextAwareProfile::ContextAwareProfile(QQmlContext* context) : + QQuickWebEngineProfile(context), _context(context) { } -void ContextAwareProfile::restrictContext(QQmlContext* context) { - context->setContextProperty(RESTRICTED_FLAG_PROPERTY, true); +void ContextAwareProfile::restrictContext(QQmlContext* context, bool restrict) { + context->setContextProperty(RESTRICTED_FLAG_PROPERTY, restrict); } -bool ContextAwareProfile::isRestricted(QQmlContext* context) { - return context->contextProperty(RESTRICTED_FLAG_PROPERTY).toBool(); +bool ContextAwareProfile::isRestricted() { + if (QThread::currentThread() != thread()) { + bool restrictedResult = false; + BLOCKING_INVOKE_METHOD(this, "isRestricted", Q_RETURN_ARG(bool, restrictedResult)); + } + return _context->contextProperty(RESTRICTED_FLAG_PROPERTY).toBool(); } #endif \ No newline at end of file diff --git a/libraries/ui/src/ui/types/ContextAwareProfile.h b/libraries/ui/src/ui/types/ContextAwareProfile.h index 8fa5b98878..ad42c2677d 100644 --- a/libraries/ui/src/ui/types/ContextAwareProfile.h +++ b/libraries/ui/src/ui/types/ContextAwareProfile.h @@ -20,16 +20,16 @@ class QQmlContext; class ContextAwareProfile : public QQuickWebEngineProfile { + Q_OBJECT public: - static void restrictContext(QQmlContext* context); - static bool isRestricted(QQmlContext* context); - QQmlContext* getContext() const { return _context; } + static void restrictContext(QQmlContext* context, bool restrict = true); + Q_INVOKABLE bool isRestricted(); protected: class RequestInterceptor : public QWebEngineUrlRequestInterceptor { public: RequestInterceptor(ContextAwareProfile* parent) : QWebEngineUrlRequestInterceptor(parent), _profile(parent) {} - QQmlContext* getContext() const { return _profile->getContext(); } + bool isRestricted() { return _profile->isRestricted(); } protected: ContextAwareProfile* _profile; }; diff --git a/libraries/ui/src/ui/types/FileTypeProfile.cpp b/libraries/ui/src/ui/types/FileTypeProfile.cpp index 073460903e..615e80b85c 100644 --- a/libraries/ui/src/ui/types/FileTypeProfile.cpp +++ b/libraries/ui/src/ui/types/FileTypeProfile.cpp @@ -29,8 +29,8 @@ FileTypeProfile::FileTypeProfile(QQmlContext* parent) : } void FileTypeProfile::RequestInterceptor::interceptRequest(QWebEngineUrlRequestInfo& info) { - RequestFilters::interceptHFWebEngineRequest(info, getContext()); - RequestFilters::interceptFileType(info, getContext()); + RequestFilters::interceptHFWebEngineRequest(info, isRestricted()); + RequestFilters::interceptFileType(info); } void FileTypeProfile::registerWithContext(QQmlContext* context) { diff --git a/libraries/ui/src/ui/types/HFWebEngineProfile.cpp b/libraries/ui/src/ui/types/HFWebEngineProfile.cpp index ef1d009f09..3c5701cc52 100644 --- a/libraries/ui/src/ui/types/HFWebEngineProfile.cpp +++ b/libraries/ui/src/ui/types/HFWebEngineProfile.cpp @@ -28,7 +28,7 @@ HFWebEngineProfile::HFWebEngineProfile(QQmlContext* parent) : Parent(parent) } void HFWebEngineProfile::RequestInterceptor::interceptRequest(QWebEngineUrlRequestInfo& info) { - RequestFilters::interceptHFWebEngineRequest(info, getContext()); + RequestFilters::interceptHFWebEngineRequest(info, isRestricted()); } void HFWebEngineProfile::registerWithContext(QQmlContext* context) { diff --git a/libraries/ui/src/ui/types/RequestFilters.cpp b/libraries/ui/src/ui/types/RequestFilters.cpp index a3b3b7dc57..3fa1cd0bd9 100644 --- a/libraries/ui/src/ui/types/RequestFilters.cpp +++ b/libraries/ui/src/ui/types/RequestFilters.cpp @@ -63,8 +63,8 @@ namespace { } } -void RequestFilters::interceptHFWebEngineRequest(QWebEngineUrlRequestInfo& info, QQmlContext* context) { - if (ContextAwareProfile::isRestricted(context) && blockLocalFiles(info)) { +void RequestFilters::interceptHFWebEngineRequest(QWebEngineUrlRequestInfo& info, bool restricted) { + if (restricted && blockLocalFiles(info)) { return; } @@ -90,7 +90,7 @@ void RequestFilters::interceptHFWebEngineRequest(QWebEngineUrlRequestInfo& info, info.setHttpHeader(USER_AGENT.toLocal8Bit(), tokenString.toLocal8Bit()); } -void RequestFilters::interceptFileType(QWebEngineUrlRequestInfo& info, QQmlContext* context) { +void RequestFilters::interceptFileType(QWebEngineUrlRequestInfo& info) { QString filename = info.requestUrl().fileName(); if (isScript(filename) || isJSON(filename)) { static const QString CONTENT_HEADER = "Accept"; diff --git a/libraries/ui/src/ui/types/RequestFilters.h b/libraries/ui/src/ui/types/RequestFilters.h index 8fde94a1b4..0c25bbb354 100644 --- a/libraries/ui/src/ui/types/RequestFilters.h +++ b/libraries/ui/src/ui/types/RequestFilters.h @@ -24,8 +24,8 @@ class QQmlContext; class RequestFilters : public QObject { public: - static void interceptHFWebEngineRequest(QWebEngineUrlRequestInfo& info, QQmlContext* context); - static void interceptFileType(QWebEngineUrlRequestInfo& info, QQmlContext* context); + static void interceptHFWebEngineRequest(QWebEngineUrlRequestInfo& info, bool restricted); + static void interceptFileType(QWebEngineUrlRequestInfo& info); }; #endif From 5a6e2e5daf56947f99b20ae58078007d3d7cb14e Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Wed, 28 Aug 2019 10:13:03 -0700 Subject: [PATCH 2/4] Add caching to the restricted value --- .../ui/src/ui/types/ContextAwareProfile.cpp | 23 +++++++++++++++---- .../ui/src/ui/types/ContextAwareProfile.h | 9 ++++++-- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/libraries/ui/src/ui/types/ContextAwareProfile.cpp b/libraries/ui/src/ui/types/ContextAwareProfile.cpp index 8e363e6546..ee47435d4d 100644 --- a/libraries/ui/src/ui/types/ContextAwareProfile.cpp +++ b/libraries/ui/src/ui/types/ContextAwareProfile.cpp @@ -10,13 +10,15 @@ // #include "ContextAwareProfile.h" -#include -#include - #if !defined(Q_OS_ANDROID) +#include #include +#include +#include + + static const QString RESTRICTED_FLAG_PROPERTY = "RestrictFileAccess"; ContextAwareProfile::ContextAwareProfile(QQmlContext* context) : @@ -27,12 +29,23 @@ void ContextAwareProfile::restrictContext(QQmlContext* context, bool restrict) { context->setContextProperty(RESTRICTED_FLAG_PROPERTY, restrict); } -bool ContextAwareProfile::isRestricted() { +bool ContextAwareProfile::isRestrictedInternal() { if (QThread::currentThread() != thread()) { bool restrictedResult = false; - BLOCKING_INVOKE_METHOD(this, "isRestricted", Q_RETURN_ARG(bool, restrictedResult)); + BLOCKING_INVOKE_METHOD(this, "isRestrictedInternal", Q_RETURN_ARG(bool, restrictedResult)); + return restrictedResult; } return _context->contextProperty(RESTRICTED_FLAG_PROPERTY).toBool(); } +bool ContextAwareProfile::isRestricted() { + auto now = usecTimestampNow(); + auto cacheAge = now - _lastCacheCheck; + if (cacheAge > MAX_CACHE_AGE) { + _cachedValue = isRestrictedInternal(); + _lastCacheCheck = now; + } + return _cachedValue; +} + #endif \ No newline at end of file diff --git a/libraries/ui/src/ui/types/ContextAwareProfile.h b/libraries/ui/src/ui/types/ContextAwareProfile.h index ad42c2677d..b55f95c180 100644 --- a/libraries/ui/src/ui/types/ContextAwareProfile.h +++ b/libraries/ui/src/ui/types/ContextAwareProfile.h @@ -16,6 +16,7 @@ #if !defined(Q_OS_ANDROID) #include #include +#include class QQmlContext; @@ -23,7 +24,8 @@ class ContextAwareProfile : public QQuickWebEngineProfile { Q_OBJECT public: static void restrictContext(QQmlContext* context, bool restrict = true); - Q_INVOKABLE bool isRestricted(); + bool isRestricted(); + Q_INVOKABLE bool isRestrictedInternal(); protected: class RequestInterceptor : public QWebEngineUrlRequestInterceptor { @@ -35,7 +37,10 @@ protected: }; ContextAwareProfile(QQmlContext* parent); - QQmlContext* _context; + QQmlContext* _context{ nullptr }; + bool _cachedValue{ false }; + quint64 _lastCacheCheck{ 0 }; + static const quint64 MAX_CACHE_AGE = MSECS_PER_SECOND; }; #endif From af22ab55bbf8a136ac5f87d7dff3e6b0cab620f7 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Thu, 29 Aug 2019 08:46:46 -0700 Subject: [PATCH 3/4] PR feedback --- .../ui/src/ui/types/ContextAwareProfile.cpp | 14 ++++++------- .../ui/src/ui/types/ContextAwareProfile.h | 21 +++++++++++++------ 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/libraries/ui/src/ui/types/ContextAwareProfile.cpp b/libraries/ui/src/ui/types/ContextAwareProfile.cpp index ee47435d4d..f0b06f2b21 100644 --- a/libraries/ui/src/ui/types/ContextAwareProfile.cpp +++ b/libraries/ui/src/ui/types/ContextAwareProfile.cpp @@ -10,19 +10,20 @@ // #include "ContextAwareProfile.h" -#if !defined(Q_OS_ANDROID) +#include #include #include #include #include - static const QString RESTRICTED_FLAG_PROPERTY = "RestrictFileAccess"; ContextAwareProfile::ContextAwareProfile(QQmlContext* context) : - QQuickWebEngineProfile(context), _context(context) { } + QQuickWebEngineProfile(context), _context(context) { + assert(context); +} void ContextAwareProfile::restrictContext(QQmlContext* context, bool restrict) { @@ -40,12 +41,9 @@ bool ContextAwareProfile::isRestrictedInternal() { bool ContextAwareProfile::isRestricted() { auto now = usecTimestampNow(); - auto cacheAge = now - _lastCacheCheck; - if (cacheAge > MAX_CACHE_AGE) { + if (now > _cacheExpiry) { _cachedValue = isRestrictedInternal(); - _lastCacheCheck = now; + _cacheExpiry = now + MAX_CACHE_AGE; } return _cachedValue; } - -#endif \ No newline at end of file diff --git a/libraries/ui/src/ui/types/ContextAwareProfile.h b/libraries/ui/src/ui/types/ContextAwareProfile.h index b55f95c180..3192d2be54 100644 --- a/libraries/ui/src/ui/types/ContextAwareProfile.h +++ b/libraries/ui/src/ui/types/ContextAwareProfile.h @@ -16,11 +16,21 @@ #if !defined(Q_OS_ANDROID) #include #include + +using ContextAwareProfileParent = QQuickWebEngineProfile; +using RequestInterceptorParent = QWebEngineUrlRequestInterceptor; +#else +#include + +using ContextAwareProfileParent = QObject; +using RequestInterceptorParent = QObject; +#endif + #include class QQmlContext; -class ContextAwareProfile : public QQuickWebEngineProfile { +class ContextAwareProfile : public ContextAwareProfileParent { Q_OBJECT public: static void restrictContext(QQmlContext* context, bool restrict = true); @@ -28,9 +38,9 @@ public: Q_INVOKABLE bool isRestrictedInternal(); protected: - class RequestInterceptor : public QWebEngineUrlRequestInterceptor { + class RequestInterceptor : public RequestInterceptorParent { public: - RequestInterceptor(ContextAwareProfile* parent) : QWebEngineUrlRequestInterceptor(parent), _profile(parent) {} + RequestInterceptor(ContextAwareProfile* parent) : RequestInterceptorParent(parent), _profile(parent) { } bool isRestricted() { return _profile->isRestricted(); } protected: ContextAwareProfile* _profile; @@ -39,9 +49,8 @@ protected: ContextAwareProfile(QQmlContext* parent); QQmlContext* _context{ nullptr }; bool _cachedValue{ false }; - quint64 _lastCacheCheck{ 0 }; - static const quint64 MAX_CACHE_AGE = MSECS_PER_SECOND; + quint64 _cacheExpiry{ 0 }; + constexpr static quint64 MAX_CACHE_AGE = MSECS_PER_SECOND; }; -#endif #endif // hifi_FileTypeProfile_h From 36be00bf97e5d7edf4aea967092c2c1f34727c12 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Thu, 29 Aug 2019 10:25:57 -0700 Subject: [PATCH 4/4] Fix android build again --- libraries/ui/src/ui/types/ContextAwareProfile.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/ui/src/ui/types/ContextAwareProfile.cpp b/libraries/ui/src/ui/types/ContextAwareProfile.cpp index f0b06f2b21..5efeba0602 100644 --- a/libraries/ui/src/ui/types/ContextAwareProfile.cpp +++ b/libraries/ui/src/ui/types/ContextAwareProfile.cpp @@ -21,7 +21,7 @@ static const QString RESTRICTED_FLAG_PROPERTY = "RestrictFileAccess"; ContextAwareProfile::ContextAwareProfile(QQmlContext* context) : - QQuickWebEngineProfile(context), _context(context) { + ContextAwareProfileParent(context), _context(context) { assert(context); }