1823770 - The browser crashes when I put view-source:view-source:

Status: ASSIGNED

(Firefox for Android :: Browser Engine, defect, P3)

Description

[Judemont]

Steps to reproduce:

Write in the address bar "view-source:view-source:" + the url of any website

Actual results:

Firefox closes.

Expected results:

A search with the search engine or an error message.

Comment 1

[jackyzy823]

The setting "Homepage -> Opening screen -> Last tab" make this situation worse.

The view-source recursion should be prevented.

Reference: function fixupViewSource in https://searchfox.org/mozilla-central/source/docshell/base/URIFixup.sys.mjs#1131

Comment 2

[jackyzy823]

Here's some log you may concern

02.800 D GeckoViewModule: dispatch GeckoView:LoadUri, data={"uri":"view-source:view-source:https://www.xxx.xxx","flags":0,"headerFilter":1}
02.802 D GeckoViewNavigation: onEvent: event=GeckoView:LoadUri, data={"uri":"view-source:view-source:https://www.xxx.xxx","flags":0,"headerFilter":1}
02.808 E GeckoConsole: [JavaScript Error: "NS_ERROR_FAILURE: Prevent view-source recursion" {file: "resource://gre/modules/URIFixup.sys.mjs" line: 1144}]
02.808 E GeckoConsole: fixupViewSource@resource://gre/modules/URIFixup.sys.mjs:1144:11
02.808 E GeckoConsole: getFixupURIInfo@resource://gre/modules/URIFixup.sys.mjs:312:55
02.808 E GeckoConsole: fixupAndLoadURIString@resource://gre/modules/RemoteWebNavigation.sys.mjs:136:31
02.808 E GeckoConsole: fixupAndLoadURIString/<@chrome://global/content/elements/browser-custom-element.js:844:28
02.808 E GeckoConsole: _wrapURIChangeCall@chrome://global/content/elements/browser-custom-element.js:772:9
02.808 E GeckoConsole: fixupAndLoadURIString@chrome://global/content/elements/browser-custom-element.js:843:12
02.808 E GeckoConsole: onEvent@resource://gre/modules/GeckoViewNavigation.sys.mjs:261:22
02.808 E GeckoConsole: _dispatch@resource://gre/modules/GeckoViewModule.sys.mjs:149:21
02.808 E GeckoConsole: onEvent@resource://gre/modules/GeckoViewModule.sys.mjs:137:12
02.808 E GeckoConsole: [JavaScript Error: "NS_ERROR_FAILURE: Prevent view-source recursion" {file: "resource://gre/modules/URIFixup.sys.mjs" line: 1144}]
02.808 E GeckoConsole: fixupViewSource@resource://gre/modules/URIFixup.sys.mjs:1144:11
02.808 E GeckoConsole: getFixupURIInfo@resource://gre/modules/URIFixup.sys.mjs:312:55
02.808 E GeckoConsole: fixupAndLoadURIString@resource://gre/modules/RemoteWebNavigation.sys.mjs:154:35
02.808 E GeckoConsole: fixupAndLoadURIString/<@chrome://global/content/elements/browser-custom-element.js:844:28
02.808 E GeckoConsole: _wrapURIChangeCall@chrome://global/content/elements/browser-custom-element.js:772:9
02.808 E GeckoConsole: fixupAndLoadURIString@chrome://global/content/elements/browser-custom-element.js:843:12
02.808 E GeckoConsole: onEvent@resource://gre/modules/GeckoViewNavigation.sys.mjs:261:22
02.808 E GeckoConsole: _dispatch@resource://gre/modules/GeckoViewModule.sys.mjs:149:21
02.808 E GeckoConsole: onEvent@resource://gre/modules/GeckoViewModule.sys.mjs:137:12
02.809 D GeckoViewModule: dispatch GeckoView:SetPriorityHint, data={"priorityHint":1}
02.811 D GeckoViewContent: onEvent: event=GeckoView:SetPriorityHint, data={"priorityHint":1}
02.813 D GeckoViewProgress: ProgressTracker onStateChange: isTopLevel=true, flags=0xf0001, status=NS_OK
02.813 D GeckoViewProgress: ProgressTracker onStateChange: uri=view-source:view-source:https://www.xxx.xxx/
02.813 D GeckoViewProgress: ProgressTracker start view-source:view-source:https://www.xxx.xxx/
02.813 D GeckoViewProgress: ProgressTracker updateProgress
02.814 D GeckoViewProgress: ProgressTracker updateProgress data={"prev":0,"uri":"view-source:view-source:https://www.xxx.xxx/","locationChange":false,"pageStart":true,"pageStop":false,"firstPaint":false,"pageShow":false,"parsed":false} progress=15
02.814 D GeckoViewProgress: StateTracker onStateChange: isTopLevel=true, flags=0xf0001, status=NS_OK loadType=1
02.817 D GeckoViewModule: dispatch GeckoView:SetActive, data={"active":true}
02.817 D GeckoViewContent: onEvent: event=GeckoView:SetActive, data={"active":true}
02.817 D GeckoViewXUL: onEvent GeckoView:UpdateModuleState {"enabled":true,"module":"GeckoViewSelectionAction"}
02.818 D GeckoViewSelectionAction: onEnable
02.818 I WebExtension: releasePendingMessages: extension=[e-mail] nativeApp=mozacWebchannel session=org.mozilla.geckoview.GeckoSession@b06fb78
02.818 D GeckoViewModule: registerListener ["GeckoView:ExecuteSelectionAction"]
02.818 I WebExtension: releasePendingMessages: extension=[e-mail] nativeApp=mozacReaderviewActive session=org.mozilla.geckoview.GeckoSession@b06fb78
02.818 I WebExtension: releasePendingMessages: extension=[e-mail] nativeApp=mozacReaderview session=org.mozilla.geckoview.GeckoSession@b06fb78
02.819 D GeckoViewXUL: onEvent GeckoView:UpdateModuleState {"enabled":true,"module":"GeckoViewPrint"}
02.822 D GeckoViewXUL: onEvent GeckoView:UpdateModuleState {"enabled":true,"module":"GeckoViewSelectionAction"}
02.830 D GeckoSession: handleMessage GeckoView:ProgressChanged uri=null
02.830 D GeckoSession: handleMessage GeckoView:PageStart uri=view-source:view-source:https://www.xxx.xxx/
02.830 I GeckoSession: handleMessage GeckoView:PageStart uri=
03.384 I Gecko : Exiting due to channel error.
03.384 W InputDispatcher: channel '1fc4205 org.mozilla.fenix.debug/org.mozilla.fenix.debug.App (server)' ~ Consumer closed input channel or an error occurred. events=0x9
03.384 E InputDispatcher: channel '1fc4205 org.mozilla.fenix.debug/org.mozilla.fenix.debug.App (server)' ~ Channel is unrecoverably broken and will be disposed!
03.385 E SurfaceFlinger: Failed to find layer (SurfaceView - org.mozilla.fenix.debug/org.mozilla.fenix.debug.App#0) in layer parent (no-parent).
03.385 I Zygote : Process 17887 exited cleanly (11)
03.386 I ActivityManager: Process org.mozilla.fenix.debug (pid 17887) has died: fore TOP

Will not catching error from fixupViewSource cause Gecko exit?

Comment 3

[Chris Peterson [:cpeterson]]

Thanks for the bug report and log. We should prevent view-source of view-source:, though this is a corner case that few users will run into.

Comment 4

[jackyzy823]

It is more then just a corner case. It is about how fenix/GeckoView handling error from Gecko core part.

A crash of the whole application is not acceptable. It would be better if fenix could behave like the desktop version which only crashes a tab (Gah. Your tab just crashed) , not the whole program.

Comment 5

[jackyzy823]

After some more digging, i found it is a regression :Fenix 108.2 will not crash , but the prograss bar stop at 15% and page is blank. Fenix 109.1.1 crashes.

This line cause the crash and the comment above pointing to https://bugzilla.mozilla.org/show_bug.cgi?id=1815509 and then further pointing to https://bugzilla.mozilla.org/show_bug.cgi?id=1810141

Comment 6

[:Gijs (he/him)]

I don't understand why I was needinfo'd here. The refactors mentioned shouldn't be causing crashes, and didn't introduce the code that throws the error mentioned in comment 2 - that error will almost certainly have been there before.

It's not really clear to me from the comments here whether this crash happens on non-debug builds as well.

Can someone post an actual crash trace or a link to crash-stats for a crash corresponding to this bug?

Comment 7

[:Gijs (he/him)]

(In reply to jackyzy823 from comment #4)

behave like the desktop version which only crashes a tab (Gah. Your tab just crashed) , not the whole program.

FWIW I cannot reproduce this either. If I put view-source:view-source:https://mozilla.org/ in the address bar and hit enter, a web search page from the default search provider loads.

Comment 8

[jackyzy823]

It's not really clear to me from the comments here whether this crash happens on non-debug builds as well.

Again . Fenix 108.2 will not crash , but the prograss bar stop at 15% and page is blank. Fenix 109.1.1 crashes. They are both non-debug the RELEASE build.

I don't understand why I was needinfo'd here.

Because the comment above the line which i think it causes crash leads me to someone maybe the codeowner.

And i change that line from this._browser.browsingContext.fixupAndLoadURIString(uriString, { to old version this._browser.browsingContext.loadURI(uri, { then no crash happens.

I feel sorry if this bothers you.

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

(In reply to jackyzy823 from comment #4)

behave like the desktop version which only crashes a tab (Gah. Your tab just crashed) , not the whole program.

FWIW I cannot reproduce this either. If I put view-source:view-source:https://mozilla.org/ in the address bar and hit enter, a web search page from the default search provider loads.

This comment is for a general purpose to handle Gecko Core's error properly instead of crashing.

Can someone post an actual crash trace or a link to crash-stats for a crash corresponding to this bug?

If you may concern, https://crash-stats.mozilla.org/report/index/cdccaf86-00bc-4f08-801f-9bd750230412

Comment 9

[:Gijs (he/him)]

(In reply to jackyzy823 from comment #8)

It's not really clear to me from the comments here whether this crash happens on non-debug builds as well.

Again . Fenix 108.2 will not crash , but the prograss bar stop at 15% and page is blank. Fenix 109.1.1 crashes. They are both non-debug the RELEASE build.

Thank you for clarifying. My confusion came from the fact that the log you pasted indicates "debug" in several places.

Because the comment above the line which i think it causes crash leads me to someone maybe the codeowner.

I don't own any Fenix code, or any http code, which is what's crashing here. I changed the code that causes some of the errors in the log, but it is as yet unclear if or how that is in any way related to the crashes.

And i change that line from this._browser.browsingContext.fixupAndLoadURIString(uriString, { to old version this._browser.browsingContext.loadURI(uri, { then no crash happens.

That would also, in some circumstances break the load if the input in the address bar is not a URL, but something that gets transformed into a search query. We're not guaranteed to have a uri object at that point.

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

(In reply to jackyzy823 from comment #4)

behave like the desktop version which only crashes a tab (Gah. Your tab just crashed) , not the whole program.

FWIW I cannot reproduce this either. If I put view-source:view-source:https://mozilla.org/ in the address bar and hit enter, a web search page from the default search provider loads.

This comment is for a general purpose to handle Gecko Core's error properly instead of crashing.

My point is that I cannot reproduce a tab crash on desktop Firefox. Your crash report link (thanks) points to a mobile crash. Can you also link to a crash report for the desktop tab crash you're seeing?

Can someone post an actual crash trace or a link to crash-stats for a crash corresponding to this bug?

If you may concern, https://crash-stats.mozilla.org/report/index/cdccaf86-00bc-4f08-801f-9bd750230412

Thanks. This points to core networking C++ code. Valentin, do you know what's happening here? Is mChannel perhaps null, and if so, how is that possible? ( https://hg.mozilla.org/mozilla-central/file/948cf466f3f2e0db976af227c20d222a971d8293/netwerk/protocol/http/HttpChannelParent.cpp#l945 )

Comment 10

[jackyzy823]

My point is that I cannot reproduce a tab crash on desktop Firefox. Your crash report link (thanks) points to a mobile crash. Can you also link to a crash report for the desktop tab crash you're seeing?

I don't mean that view-source:view-source: will cause a crash in desktop version. It just work as you and reporter said: A search with the search engine

We just expect the mobile version to not crash totally in this situation . It could be fixed to A search with the search engine or show invalid address or at least a single tab crash.

Anyway thanks very much for your kind help.

Comment 11

[Adina Petridean QA [:apetridean]]

I was able to reproduce this issue on the RC 116.3.0 build, on Beta 117.0b9, and on Nightly 118.0a1 from 8/23.

Tested with Google Pixel 7 Pro ( Android 14) and Motorola Moto G9 plus (Android 11).

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1823770 - Fix android crash when loading view-source:view-source:http://example.com r=Gijs

This is a simple patch that fixes the crash on optimized builds.
The assertions would still be triggered on debug builds.

There are other issues to follow-up on, such as CreateTruncatedPrincipal
failing as it's using a hardcoded separator :// which leads the URL
to failing to parse when transformed into view-source:view-source://http://example.com

Comment 13

[Valentin Gosu [:valentin] (he/him)]

It was initially apparent that this bug only reproduces on Android, however, upon further inspection it seems it also happens on Desktop but it's hidden by the URIFixup code:
https://searchfox.org/mozilla-central/rev/8f09f6a6ff0d8b6ea75a1e1279a06ba02479578c/docshell/base/URIFixup.sys.mjs#1147-1152

if (innerScheme == "view-source") {
  throw new Components.Exception(
    "Prevent view-source recursion",
    Cr.NS_ERROR_FAILURE
  );
}

When this code is exercised an exception is thrown. The URL is considered to be invalid, so the suggestion is to search it using the default search engine. If you hit enter after typing it in the URL bar, there is no crash. If instead you hit Escape, then enter you will see this crash.

If I remove the code above, there is effectively no difference between the Desktop and GeckoView crash.

After this first issue, we then hit another assertion in debug builds:

https://searchfox.org/mozilla-central/rev/8f09f6a6ff0d8b6ea75a1e1279a06ba02479578c/netwerk/base/LoadInfo.cpp#1499,1503-1541

nsAutoCString separator("://");
...
if (aPrincipal->SchemeIs("view-source")) {
  // The path portion of the view-source URI will be the URI whose source is
  // being viewed, so we create a new URI object with a truncated form of
  // the path and append the view-source scheme to the front again.
  nsAutoCString viewSourcePath;
  aPrincipal->GetFilePath(viewSourcePath);

  nsCOMPtr<nsIURI> nestedURI;
  nsresult rv = NS_NewURI(getter_AddRefs(nestedURI), viewSourcePath);

  if (NS_FAILED(rv)) {
    // Since the path here should be an already validated URI this should
    // never happen.
    NS_WARNING(viewSourcePath.get());
    MOZ_ASSERT(false,
               "Failed to create truncated form of URI with NS_NewURI.");
    truncatedPrincipal = aPrincipal;
    return truncatedPrincipal.forget();
  }

  nestedURI->GetScheme(scheme);
  nestedURI->GetHostPort(hostPort);
  nestedURI->GetFilePath(path);
  uriString += "view-source:";
} else {
...
}
uriString += scheme + separator + hostPort + path;

nsCOMPtr<nsIURI> truncatedURI;
nsresult rv = NS_NewURI(getter_AddRefs(truncatedURI), uriString);
if (NS_FAILED(rv)) {
  NS_WARNING(uriString.get());
  MOZ_ASSERT(false,
             "Failed to create truncated form of URI with NS_NewURI."); <-- HERE!
  truncatedPrincipal = aPrincipal;
  return truncatedPrincipal.forget();

When computing the principal, we use :// separator when constructing the URI string, which turns the view-source:view-source:http://example.com URL to view-source:view-source://http://example.com - which fails to parse.

We could avoid this assertion by either setting the separator to : when the scheme is view-source, or by making nsViewSourceHandler::CreateNewURI handle it properly:
https://searchfox.org/mozilla-central/rev/8f09f6a6ff0d8b6ea75a1e1279a06ba02479578c/netwerk/protocol/viewsource/nsViewSourceHandler.cpp#64-69

int32_t colon = aSpec.FindChar(':');
if (colon == kNotFound) return NS_ERROR_MALFORMED_URI;

nsCOMPtr<nsIURI> innerURI;
nsresult rv = NS_NewURI(getter_AddRefs(innerURI), Substring(aSpec, colon + 1),
                        aCharset, aBaseURI);

What happens then, is that we attempt the load and we hit this part of the code HttpChannelParent::ConnectChannel:
https://searchfox.org/mozilla-central/rev/8f09f6a6ff0d8b6ea75a1e1279a06ba02479578c/netwerk/protocol/http/HttpChannelParent.cpp#680-684

if (!mChannel) {
  LOG(("  but it's not HttpBaseChannel"));
  Delete();
  return true;
}

which causes us to close the IPC channel, but more importantly mChannel stays null, and mPromise stays empty.

Following that we get a call to HttpChannelParent::ContinueVerification coming from DocumentLoadListener::RedirectToRealChannelFinished - which triggers an assertion or crash on a null mChannel.

---

The quick I posted in D187092 fix solves the crash on non-debug builds by performing a null check.
I guess we'd ideally also fix the other issues identified here, and add this to the test suite.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

The actual crash will probably get fixed in bug 1840160.
Keeping this bug to add tests and also to fix the several issues I listed in my previous comment.