Properly handle QML that failed to load

[Cimbali]

Description

Added early bail-out when qml fails to load in UI command, since the current callback and proper exit only get called too late (in qApp.exec()), i.e. after the error caused a segfault.

An alternate fix would be to delay all connect() calls that require the window (like the one below) until after the execution has started and the window has been confirmed to exist.

https://github.com/mozilla-mobile/mozilla-vpn-client/blob/3ae9aea3c262c0aa7a8d313976f80f38dcc957e5/src/systemtraynotificationhandler.cpp#L79-L80

Reference

See #3749

Checklist

• [ ] My code follows the style guidelines for this project I was unable to find style guidelines in the README, in the wiki, in other top-level md files in the repo, on the support website…
• [x] I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
• [x] I have performed a self review of my own code
• [x] I have commented my code PARTICULARLY in hard to understand areas
• [ ] I have added thorough tests where needed A test that the code does not segfault on failed QML loading would be nice I suppose, but I don’t know much about where or how to add it.

[firefoxci-taskcluster[bot]]

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

[codecov-commenter]

Codecov Report

Merging #4121 (39a677d) into main (3ae9aea) will decrease coverage by 3.98%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #4121      +/-   ##
==========================================
- Coverage   31.54%   27.55%   -3.99%     
==========================================
  Files         284      211      -73     
  Lines       17445    11979    -5466     
  Branches     9344     6769    -2575     
==========================================
- Hits         5503     3301    -2202     
+ Misses       5413     3944    -1469     
+ Partials     6529     4734    -1795     

Flag	Coverage Δ
functional_tests	?
lottie_tests	56.33% <ø> (ø)
qml_tests	9.59% <ø> (ø)
unit_tests	26.21% <0.00%> (-0.01%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/qmlengineholder.cpp	18.91% <0.00%> (-43.25%)	:arrow_down:
src/qmlengineholder.h	100.00% <ø> (ø)
src/pinghelper.h	0.00% <0.00%> (-100.00%)	:arrow_down:
src/pingsender.h	0.00% <0.00%> (-100.00%)	:arrow_down:
src/signature.cpp	0.00% <0.00%> (-100.00%)	:arrow_down:
src/composerblock.h	0.00% <0.00%> (-100.00%)	:arrow_down:
src/addons/addontutorial.h	0.00% <0.00%> (-100.00%)	:arrow_down:
src/tasks/release/taskrelease.h	0.00% <0.00%> (-100.00%)	:arrow_down:
src/platforms/dummy/dummypingsender.cpp	0.00% <0.00%> (-77.78%)	:arrow_down:
src/pinghelper.cpp	0.00% <0.00%> (-71.43%)	:arrow_down:
... and 165 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

[bakulf]

Can you please tell me more about what you want to achieve here? Most of the cases, the app should be able to load the main QML file because it's a QRC resource, embedded in the app. Do you have a particular scenario?

[Cimbali]

Yes @bakulf the scenario is reported in #3749.

When there is an error in loading the QML file (in this case because of a missing dependency), the app.qExec() is never reached because accessing the window (QmlEngineHolder::instance()->window() called from NotificationHandler::create(…)) causes a segfault.

The callback currently handling the loading error is never executed because of this segfault (it’s just queued to be run in the main loop), it’s dead code.

This PR fixes that by handling the error without callbacks and before calling QmlEngineHolder::window().

[Cimbali]

Closing in favour of #5237.