1627644 - typing page-icon: URL into address bar crashes Firefox

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P1)

Description

[Frederik Braun [:freddy]]

STR on nightly:

1. open new tab
2. paste page-icon:https://example.com
3. crash

Example Crash Report at https://crash-stats.mozilla.org/report/index/7b915b86-8e87-46f5-8fae-224630200406

Note how we do not allow typing moz-icon URLs into the address bar. Maybe we want the same for page-icon?

Comment 1

[Marco Bonardo [:mak]]

it doesn't crash in 74.0.0.1 for me.
Maybe this regressed with the protocol conversion to cpp? Marking that as initial guess, having a proper regrange would help.

Fwiw, the protocol is not intended for content, chrome usage should be fine, it would be fine to unify the behavior with moz-icon.

Comment 2

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #1)

having a proper regrange would help

looking

Comment 3

[:Gijs (he/him)]

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4a95f898cab860303e46df90d25f48e3f909bf5a&tochange=40ca68bfea62e27f6934d4327f1ff88df9c17828

So yeah, this looks like it's the pageicon handler refactor. That said, the previous behaviour was a blank tab with the icon in the tabstrip just spinning forever so it's never been very usable...

Comment 4

[:Gijs (he/him)]

Looks like we end up with a null loading principal (makes sense, there's a triggering principal instead, the toplevel load in the tab won't have a loading principal, just a triggering principal) and that nullptr-crashes. Christoph, can you sanity-check that that sounds right? Where is the loading principal supposed to come from in this case (see stack in https://crash-stats.mozilla.org/report/index/7b915b86-8e87-46f5-8fae-224630200406 )?

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

From what I see we are overwriting the channel's initial loadinfo anyway. So what we should do is, create a new nullPrincipal

nsCOMPtr<nsIPrincipal> nullPrincipal = NullPrincipal::CreateWithoutOriginAttributes();

and use that for creating the channel instead of aLoadInfo->LoadingPrincipal())

and then write underneath we are calling

channel->SetLoadInfo(aLoadInfo);

and we should be good.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

If LoadInfo::LoadingPrincipal can return null, which it can, it shouldn't be called LoadingPrincipal should be called GetLoadingPrincipal instead, per the usual convention, fwiw...

Comment 7

[Emilio Cobos Álvarez [:emilio]]

FWIW, in 74 the tab crashes on a debug build with this assertion too: https://searchfox.org/mozilla-central/rev/c2bc259414706ef4be5d13df1344eebb7507a51b/toolkit/components/places/Database.cpp#371

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1627644 - Handle null loading principal in page-icon protocol handler. r=mak

This restores previous behavior. Will file a bug for the debug-build crash and
so on.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

So this restores behavior, but comment 7 is something else to look at... We don't want to instantiate the favicon service on content processes... Returning an error from NewChannel just leaves the tab spinning though... Thoughts Marco?

Comment 10

[:Gijs (he/him)]

Is it possible to return an error in such a way that docshell shows a network error page, perhaps the "this protocol was not understood" error page? That's what I'd expect to happen here...

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Yeah, I tried returning NS_ERROR_UNKNOWN_PROTOCOL, but at that point it's already too late and necko just seems to barf on it :(

Comment 12

[Marco Bonardo [:mak]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #9)

So this restores behavior, but comment 7 is something else to look at... We don't want to instantiate the favicon service on content processes... Returning an error from NewChannel just leaves the tab spinning though... Thoughts Marco?

We should keep not initializing Places in content, the cost is too high.
Can we mark a protocol to be unsafe-in-content, in a way that content just thinks it's unhandled? We return URI_DANGEROUS_TO_LOAD from GetProtocolFlags, why are we still trying to load it in content?

Comment 13

[Emilio Cobos Álvarez [:emilio]]

(In reply to Marco Bonardo [:mak] from comment #12)

We should keep not initializing Places in content, the cost is too high.

To be clear, places was being initialized in-content both before and after the move to C++.

Can we mark a protocol to be unsafe-in-content, in a way that content just thinks it's unhandled? We return URI_DANGEROUS_TO_LOAD from GetProtocolFlags, why are we still trying to load it in content?

Probably because when it comes from the url bar and that uses the system principal?

Comment 14

[Marco Bonardo [:mak]]

Ok, I like Gijs suggestion, but I don't know how to do it. I would have tried what you did in NewChannel, if it doesn't work maybe it's better to ask a netwerk peer.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Yeah, it's probably a separate bug though.

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Can I get a sec rating? It's just a null-deref which needs the user to put the URI into the URL bar (we prevent content from doing these loads), so I don't think it is sec-{high,crit}, but just in case.

Comment 17

[Frederik Braun [:freddy]]

I don't see a way to trigger it from content.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/a13c047193c3ee0701ec9c2aaac5735ba173978c
https://hg.mozilla.org/mozilla-central/rev/a13c047193c3

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9138533 [details]
Bug 1627644 - Handle null loading principal in page-icon protocol handler. r=mak

Beta/Release Uplift Approval Request

• User impact if declined: Browser crash if the user types the wrong URI in the url bar.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): simple change which fixes a null deref.
• String changes made/needed: none

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9138533 [details]
Bug 1627644 - Handle null loading principal in page-icon protocol handler. r=mak

Approved for 76.0b2.

Comment 21

[Andreea Pavel [:apavel]]

Uplifted to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/882eb68a15dba651af4fa665eb9c9c1eaa7a9fce

Comment 22

[Anca Soncutean, Desktop QA]

I've reproduced this crash with Fx 76.0a1 (2020-04-06) on Windows 10 x64.
The issue is verified fixed with Fx 77.0a1 (2020-04-08) and Fx 76.0b2 on Windows 10 x64, macOS 10.15 and Ubuntu 18.04 x64.