1722328 - New wpt crashes in /url/failure.html

Status: RESOLVED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Syncing wpt PR 29666 found new crashes in CI

Affected Tests

/url/failure.html: CRASH [Gecko-android-em-7.0-x86_64-debug-geckoview, Gecko-android-em-7.0-x86_64-opt-geckoview, Gecko-android-em-7.0-x86_64-qr-debug-geckoview, Gecko-android-em-7.0-x86_64-qr-opt-geckoview, Gecko-linux1804-64-debug, Gecko-linux1804-64-opt, Gecko-linux1804-64-qr-debug, Gecko-linux1804-64-tsan-opt, Gecko-windows10-32-qr-debug], OK [Gecko-windows10-64-qr-debug, GitHub]

CI Results

Gecko CI (Treeherder)
GitHub PR Head

Notes

Getting the crash signature into these bug reports is a TODO; sorry

These updates will be on mozilla-central once bug 1720566 lands.

Note: this bug is for tracking fixing the issues and is not
owned by the wpt sync bot.

This bug is linked to the relevant tests by an annotation in
https://github.com/web-platform-tests/wpt-metadata. These annotations
can be edited using the wpt interop dashboard
https://jgraham.github.io/wptdash/

If this bug is split into multiple bugs, please also update the
annotations, otherwise we are unable to track which wpt issues are
already triaged. Resolving as duplicate or closing this issue should
be cause the bot to automatically update or remove the annotation.

Comment 1

[Tom S [:evilpie]]

I think it would be good to triage this. This seems to cause the whole browser (event the parent!) to crash. Careful crash: https://wpt.live/url/failure.html

WARNING: 'principalOrErr.isErr()', file /home/tom/projects/mozilla/dom/ipc/PermissionMessageUtils.cpp:44
IPDL protocol error: Error deserializing 'nsIPrincipal'
###!!! ASSERTION: IPDL error: "Error deserializing 'nsIPrincipal'". Intentionally crashing.: 'Error', file /home/tom/projects/mozilla/ipc/glue/ProtocolUtils.cpp:165
Hit MOZ_CRASH(IPC FatalError in the parent process!) at /home/tom/projects/mozilla/ipc/glue/ProtocolUtils.cpp:170

Comment 2

[Tom S [:evilpie]]

The error happens in PrincipalInfoToPrincipal while trying to parse the URL http://a.b.c.xn--pokxncvks/.

Comment 3

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

(In reply to Tom S [:evilpie] from comment #2)

The error happens in PrincipalInfoToPrincipal while trying to parse the URL http://a.b.c.xn--pokxncvks/.

I fixed the last of these in bug 1790287.

Comment 4

[Daniel Veditz [:dveditz]]

Tom: I don't crash when running the test on Mac. Given comment 3, does this work for you now?

Comment 5

[Tom S [:evilpie]]

I can't reproduce the crash anymore. I think we should update the WPT ini to disallow crashes: https://searchfox.org/mozilla-central/source/testing/web-platform/meta/url/failure.html.ini

Comment 6

[Thomas Wisniewski [:twisniewski]]

This isn't crashing live, but I can trivially reproduce this crash when running using mach wpt url/failure.html. I see this error:

Mozilla crash reason: Attempting to connect to non-local address! opener is [chrome://global/content/customElements.js:375:17], uri is [https://mail.google.com/favicon.ico]

This is also still appearing as a crash on wpt.fyi: https://wpt.fyi/results/url/failure.html?label=experimental&label=master&aligned

Running the test locally I see that it is trying to open mailto: links, and thus showing Firefox's popup to select an external email app, which happens to include GMail, which shows its favicon. And since the test harness intentionally crashes when trying to access the network like that, we have our crash.

Indeed, if I remove these lines to hide GMail in the default app selection, then the crash doesn't happen, and Firefox runs the test as it does normally:
https://searchfox.org/mozilla-central/source/uriloader/exthandler/HandlerList.sys.mjs#12-15

I don't know what the right way to deal with this is, however. Perhaps there is an about:config setting that we ought to use which prevents the external app handler from being shown when running web platform tests (or we need to add one)? James, what do you think?

Comment 7

[James Graham [:jgraham]]

I tried a patch with user_pref("gecko.handlerService.defaultHandlersVersion", 100); which AIUI should prevent us trying to install the default handlers (because the version is higher than the version in the file).

Results were mixed: https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=cgu7EGwVTo2EjRTWc0BHWQ.0&revision=5ad88592898565b331d5057bc0ec7df4cfbe9fb0

On Linux this seems to be enough to allow the tests to run to completion. On mac and android we're still seeing crashes trying to connect to 1.2.3.08, which is directly in the test data. On Windows we're seeing a timeout, which I guess could be something blocked on a handler dialog or similar, but it's hard to tell directly.

So I think landing this would make the test less broken, but not fixed :) In particular it might be enough for wpt.fyi to show which subtests we are passing (since that's running on Linux).

Comment 8

[Thomas Wisniewski [:twisniewski]]

I agree, James. Let's at least land a patch to keep it from crashing on Linux for now. https://treeherder.mozilla.org/jobs?repo=try&revision=6ff999c11b64fc552fcf5f754e4e8dbf6c4f1d62

Comment 9

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1722328 - Don't install default email handlers on the WPT runner to prevent undesirable remote URL accesses of the GMail favicon when mailto: is accessed; r?jgraham

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c124a8303d64
Don't install default email handlers on the WPT runner to prevent undesirable remote URL accesses of the GMail favicon when mailto: is accessed; r=jgraham

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c124a8303d64

Comment 13

[Ed Guloien [:edgul]]

Attachment: Bug 1722328 - Use defaultURI to verify data and javascript schemes with '//' during parsing. r=valentin

Comment 14

[Ed Guloien [:edgul]]

Noting that we are expecting Bug 1507354 to resolve 60/90 remaining failures in this test file.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12b6e7f8ae8e
Use defaultURI to verify data and javascript schemes with '//' during parsing. r=necko-reviewers,valentin

Comment 16

[Ed Guloien [:edgul]]

Re: Comment 14, the failures we expect to pass as a result of landing patch in Bug 1507354 contain the following:

file://example:1
file://example:test/
file://example%/
file://[example\]/
file://%43%3A
file://%43%7C
file://%43|
file://C%7C
file://%43%7C/
file://­/p 
file://%C2%AD/p
file://xn--/p

Comment 17

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/12b6e7f8ae8e

Comment 18

[Ed Guloien [:edgul]]

Assuming this patch sticks then that will be all the work needed from Necko on this, the remaining failures in url/failure.html should be resolved by Bug 1507354.