2040023 - Replace window.open + data:-URL in test_ext_tabs_captureTab.html with an alternative

Status: RESOLVED FIXED

(WebExtensions :: General, task)

Description

[Rob Wu [:robwu]]

https://searchfox.org/firefox-main/rev/6e32b4962597e60549f9fc289aa627cae508887c/toolkit/components/extensions/test/mochitest/test_ext_tabs_captureTab.html currently uses window.open() with data:-URLs. That depends on the security.data_uri.block_toplevel_data_uri_navigations to be set to false, which is set at https://searchfox.org/firefox-main/rev/6e32b4962597e60549f9fc289aa627cae508887c/testing/profiles/unittest-required/user.js#208-212 .

But that pref is being removed in bug 1895528. I would suggest to switch to AppTestDelegate.openNewForegroundTab, which works on desktop but does not on mobile because on mobile a content principal is used instead of a system principal to trigger the navigation.

I'll attach a patch that fixes this.

Comment 1

[Rob Wu [:robwu]]

After replacing window.open with AppTestDelegate.openNewForegroundTab, the test gets stuck at testCaptureVisibleTabWithActiveTab because the browser.tabs.captureVisibleTab call raises an unexpected error, due to an internal error thrown from a wgp.drawSnapshot call:

[JavaScript Error: "[Exception... "Data conversion failed because significant data would be lost" nsresult: "0x80460003 (NS_ERROR_LOSS_OF_SIGNIFICANT_DATA)" location: "<unknown>" data: no]" {file: "resource://gre/modules/ExtensionCommon.sys.mjs" line: 809}]

I pinpointed the issue to PaintFragment::Record encountering an empty rect, at https://searchfox.org/firefox-main/rev/6e32b4962597e60549f9fc289aa627cae508887c/gfx/ipc/CrossProcessPaint.cpp#67
To confirm, I added this line there:
PF_LOG("boundsDevice=[%d, %d]-[%d, %d]", boundsDevice.x, boundsDevice.y, boundsDevice.width, boundsDevice.height);

and ran ./mach test toolkit/components/extensions/test/mochitest/test_ext_tabs_captureTab.html --setenv=MOZ_LOG=CrossProcessPaint:5,PaintFragment:5,sync

which shows:

[Parent 9831: Main Thread]: D/CrossProcessPaint Starting paint. [wgp=7aa490dc00, scale=1.000000, color=(255, 255, 255, 255)]
[Parent 9831: Main Thread]: D/CrossProcessPaint Queueing paint for WindowGlobalParent(7aa490dc00).
[Child 9938: Main Thread]: D/PaintFragment boundsDevice=[0, 0]-[0, 0]
[Child 9938: Main Thread]: D/PaintFragment Empty rect to paint.
[Parent 9831: Main Thread]: D/CrossProcessPaint Dropping invalid fragment from 7aa490dc00.

For comparison, a successful run (with window.open()) has:

[Child 8954: Main Thread]: D/PaintFragment boundsDevice=[0, 0]-[800, 1136]
[Child 8954: Main Thread]: D/CrossProcessPaint Recording [browsingContext=7ac8d24e00, rect=(0, 0) x (800, 1136), scale=1.000000, color=(255, 255, 255, 255)]
[Parent 8764: Main Thread]: D/CrossProcessPaint Receiving fragment from 7a9751cc00(6442450945).

The above in its turn happens because in TestRunnerActivity, created tabs via extensions (browser.tabs.create and also AppTestDelegate.openNewForegroundTab that reuses the same logic internally), the GeckoSession does not have a display surface. For window.open this was not a problem because of the patch in bug 1467915.

To get AppTestDelegate.openNewForegroundTab to work, we have to do something similar here.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 2040023 - Fix AppTestDelegate.openNewForegroundTab and use it in test_ext_tabs_captureTab.html

Top-level navigations to data:-URL will soon only be supported when
triggered with the system principal, not via window.open().
This replaces window.open() in test_ext_tabs_captureTab.html with
AppTestDelegate.openNewForegroundTab, and updates that method to
use the system principal like desktop (otherwise it would also fail for
the same reason that window.open would fail).

This fix uncovered a pre-existing issue in TestRunnerActivity: when
AppTestDelegate.openNewForegroundTab (or browser.tabs.create) creates
a new session, it does not have a surface, which causes anything that
depends on one to fail, including the tabs.captureVisibleTab method
in this test (https://bugzilla.mozilla.org/show_bug.cgi?id=2040023#c1).
That test-only issue was fixed by adding a surface as needed.

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 2040023 - Ensure dimensions ready in tabs.capture()

The tabs.capture API relies on drawSnapshot to generate an image,
which in turn relies on the dimensions to have been propagated to the
child. This was not guaranteed before, but this patch fixes the
implementation to force layout to ensure that, and throws a meaningful
error when not.

I discovered this issue while refactoring test_ext_tabs_captureTab.html
to use AppTestDelegate.openNewForegroundTab instead of window.open().
That change removed the implicit dimensions update from window.open(),
causing the test to fail on macOS with NS_ERROR_LOSS_OF_SIGNIFICANT_DATA
errors. Forcing a layout flush with getBoundingClientRect() fixes this
specific issue.

However, the test continued to fail, now because the expected width and
height in the test were 0. This happened because the test relied on
webNavigation.onCompleted before looking up the tab, but for some
reason this event fired so quickly that the dimensions were not ready at
the time that the test looked up the tab. This issue is fixed by looking
up the actual tab state again after the tabs.captureTab() call.

The above changes are covered by the test changes of the other patch at
https://bugzilla.mozilla.org/show_bug.cgi?id=2040023#c2

Besides that, this also introduces test coverage for tabs.capture() on
background tabs. That was previously not covered, now the unit test
shows that tabs.capture() works for background tabs on desktop, but
not on Android. These pre-existing behaviors now have test coverage.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1c7e3812c1f2
https://hg.mozilla.org/mozilla-central/rev/fdb938a91e47
Fix AppTestDelegate.openNewForegroundTab and use it in test_ext_tabs_captureTab.html r=willdurand,geckoview-reviewers,ohall
https://github.com/mozilla-firefox/firefox/commit/1e8c1b38174b
https://hg.mozilla.org/mozilla-central/rev/432a34318e5d
Ensure dimensions ready in tabs.capture() r=willdurand

Comment 5

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/fdb938a91e47
https://hg.mozilla.org/mozilla-central/rev/432a34318e5d