One can call special urls like
about:crashparent
via the command line.

Many mail clients will open links in HTML mails directly with a browser, independent of the protocol. (Tested with evolution and claws-mail, doesn't work in thunderbird.)
Thus this means this can often be triggered by an email. So you can send someone an email and a click in the mail (via css this can be a link overlaying the whole mail) will lead to a browser crash.

(Looking at other browsers: Chrome has similar special URLs, but doesn't allow accessing them via the command line. If this is required for testing I recommend disabling command-line opening of about urls by default and put it behind a configuration flag.)

Related: #1507702

Snorp, can you please look into this?

Depends on D20890

Patches up.

ni?ing Gijs for https://phabricator.services.mozilla.com/D20890#566736

Responded on phab.

Backed out 2 changesets (Bug 1524992) for linting opt failure on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=998c1f756e51d9c8c983cf0585971aef40162f63

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=230776458&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/2785f1f139e8018efae1426f19af1671f52ee550

Backed out 2 changesets (bug 1524992) for multiple failures and timeout

Backout: https://hg.mozilla.org/integration/autoland/rev/5943b02bfa38cc5a3724766d0f7e896ade3eb643

Failure push:https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=android%2C7.0%2Cx86-64%2Cdebug%2Cmochitests%2Cwith%2Ce10s%2Ctest-android-em-7.0-x86_64%2Fdebug-mochitest-e10s-2%2Cm-e10s%282%29&revision=fb1d7e57e253c6922a51d436dc0796e8411b24eb

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230789353&repo=autoland&lineNumber=350

Failure log mochitest:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230790373&repo=autoland&lineNumber=2094

This failed because the PGO build opens data:text/html,<script>Quitter.quit()</script> from the command line, which is unsurprisingly not allowed for external loads. I'm not quite sure how to proceed. Gijs, do you have suggestions?

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #12)

This failed because the PGO build opens data:text/html,<script>Quitter.quit()</script> from the command line, which is unsurprisingly not allowed for external loads. I'm not quite sure how to proceed. Gijs, do you have suggestions?

The addon that provides Quitter should provide its own (moz-extension) URL that does what the data URL does here, assuming that works. Or, even better, we should run the test against the test http server and request an https URL. That'll probably provide a more realistic PGO target in terms of optimizing when we load NSS (!)

:glandium or :ted, where does that code live?

The code that launches the browser for the PGO profiling run is here. The source for the quitter extension is here, but note that due to it using an experiment API we have a signed copy of the XPI checked-in to the tree in that directory, which is what actually gets used in CI.

Yeah, updating the quitter extension is incredibly annoying because of the signing requirement. Maybe now is the time to move the PGO stuff to use Marionette? Ted, you thought this would be relatively easy in bug 1393449. Do we have anyone who could do this now? Maybe mshal?

I got a wip for using marionette in profileserver.py here: https://hg.mozilla.org/try/rev/2b675268913a36262ecb76004dd795208251e6df

So the first restart of Firefox doesn't have to be triggered from inside the application? I ask because you aren't using the in_app flag for restart().

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] (away 02/28 - 03/03) from comment #18)

So the first restart of Firefox doesn't have to be triggered from inside the application? I ask because you aren't using the in_app flag for restart().

No, I think it does need in_app. I'll update.

Backed out for failing valgrind

Push with failures https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=231744705&revision=face9d1e886872033b70fbdff5a40eaa244db3fb

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231744705&repo=autoland&lineNumber=38530

Backout: https://hg.mozilla.org/integration/autoland/rev/88ffdaa924cd625629d50fdbc97923ee9ee42b0f

smaug, I'm not sure I really understand why data: URIs would not be allowed with LOAD_NORMAL_EXTERNAL. Do you know? Is it possible to relax this restriction without breaking a bunch of other stuff? The goal here is to disallow about:crashparent from the command line by marking command line loads as external, but it's breaking data:<script>Quitter.quit()</script> used by automation.

I don't know the answer from top of my head. Is this perhaps about bug 1401895?
ckerschb might recall data: handling better.

Hmm, that bug suggests data: from command line should already be blocked, but we're clearly using it in automation. Maybe we set a pref that allows it?

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #22)

smaug, I'm not sure I really understand why data: URIs would not be allowed with LOAD_NORMAL_EXTERNAL. Do you know? Is it possible to relax this restriction without breaking a bunch of other stuff? The goal here is to disallow about:crashparent from the command line by marking command line loads as external, but it's breaking data:<script>Quitter.quit()</script> used by automation.

FWIW, the relevant code for top-level data: URI blocking is here [1]. We want to block external data: URI loads to prevent phishing attacks - imagine you click a link from within thunderbird - in that case the GetLoadTriggeredFromExternal flag is set and we block the load.

There is a pref |security.data_uri.block_toplevel_data_uri_navigations| which you can flip in case you need to allow data: URI loading in our testsuite.

[1] https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#36

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)

There is a pref |security.data_uri.block_toplevel_data_uri_navigations| which you can flip in case you need to allow data: URI loading in our testsuite.

Would it be a good idea to have such a switch as well for about-URIs?

(In reply to Hanno Boeck from comment #26)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)

There is a pref |security.data_uri.block_toplevel_data_uri_navigations| which you can flip in case you need to allow data: URI loading in our testsuite.

Would it be a good idea to have such a switch as well for about-URIs?

No. The data: stuff is mostly about principal inheritance, spoofing, and all the fun ways that data URIs were "special"; "about:blank" is a perfectly valid URL to want to open from the commandline, and "about:home" and "about:newtab" and similar might also be passed in some situations (though maybe we mostly use explicit other flags for that today, I don't know off-hand).

Anyway, we can't ban about: on the commandline point-blank. We should just stop it for these specific URLs.

(In reply to :Gijs (he/him) from comment #27)

Anyway, we can't ban about: on the commandline point-blank. We should just stop it for these specific URLs.

Okay, my question was more related to the option, not to what exactly to ban.

I just think it's valuable to have a possibility to still run the "crashing" URLs from the commandline in testing situations while blocking that option for normal installs. (I have some tests personally that make use of it).

(In reply to Hanno Boeck from comment #28)

(In reply to :Gijs (he/him) from comment #27)

Anyway, we can't ban about: on the commandline point-blank. We should just stop it for these specific URLs.

Okay, my question was more related to the option, not to what exactly to ban.

I just think it's valuable to have a possibility to still run the "crashing" URLs from the commandline in testing situations while blocking that option for normal installs. (I have some tests personally that make use of it).

Oh, I see, sorry for misreading. I hadn't considered it from that angle. I guess that's up to :snorp...

(In reply to :Gijs (he/him) from comment #29)

(In reply to Hanno Boeck from comment #28)

(In reply to :Gijs (he/him) from comment #27)

Anyway, we can't ban about: on the commandline point-blank. We should just stop it for these specific URLs.

Okay, my question was more related to the option, not to what exactly to ban.

I just think it's valuable to have a possibility to still run the "crashing" URLs from the commandline in testing situations while blocking that option for normal installs. (I have some tests personally that make use of it).

Oh, I see, sorry for misreading. I hadn't considered it from that angle. I guess that's up to :snorp...

I'm willing to do whatever. I figured treating command line urls as external (LOAD_FLAGS_FROM_EXTERNAL), then not (optionally?) not actually crashing in about:crash* would be the way to go, but maybe I'm barking up the wrong tree? If I want something more specific, should I just filter those urls out in browser.js?

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #22)

smaug, I'm not sure I really understand why data: URIs would not be allowed with LOAD_NORMAL_EXTERNAL. Do you know? Is it possible to relax this restriction without breaking a bunch of other stuff? The goal here is to disallow about:crashparent from the command line by marking command line loads as external, but it's breaking data:<script>Quitter.quit()</script> used by automation.

FWIW, the relevant code for top-level data: URI blocking is here [1]. We want to block external data: URI loads to prevent phishing attacks - imagine you click a link from within thunderbird - in that case the GetLoadTriggeredFromExternal flag is set and we block the load.

There is a pref |security.data_uri.block_toplevel_data_uri_navigations| which you can flip in case you need to allow data: URI loading in our testsuite.

[1] https://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#36

It appears then that this is not working as expected, because we're successfully using data: from the command line in testing without setting any special pref (AFAICT). This makes sense, as GetLoadTriggeredFromExternal() will return false without my patch from this bug. It seems like the solution might be to just set the pref to allow this in the test harness that need it? I'll give that a try.

We use this to tell the Quitter extension to quit in the PGO
profile generation and Valgrind tests.

We use this to tell the Quitter extension to quit in the PGO
profile generation and Valgrind tests.

https://hg.mozilla.org/mozilla-central/rev/e239591d8bf7
https://hg.mozilla.org/mozilla-central/rev/24b03e0b300b
https://hg.mozilla.org/mozilla-central/rev/51fd0edf687f
https://hg.mozilla.org/mozilla-central/rev/d7e2fa30d85f
https://hg.mozilla.org/mozilla-central/rev/7f47a4e7c43f