1678734 - Crash in [@ mozilla::extensions::StreamFilterParent::Init] MOZ_RELEASE_ASSERT(traceable)

Status: VERIFIED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Rob Wu [:robwu]]

Crash report: https://crash-stats.mozilla.org/report/index/731a6bb9-daa5-40da-b214-243250201113

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(traceable)

This means that the channel parameter passed to extensions::StreamFilterParent::Attach(channel, endpoints) is NOT a nsITraceableChannel.
This is responsible for about half of the crashes attributed to that method in the past week, 45% in the past month, 44% in the past 3 months, 35% in the past 6 months, the others are in bug 1673749 (Aggregate on "Moz crash reason" to see them).

Of the 495 crashes from the last 6 months related to this bug report, "remote type" 52% are web, 41% are extension process crashes ("remote type").

18 users left comments, 8 claim that they were using View Source (Ctrl-U) before the crash.

Top 10 frames of crashing thread:

0 xul.dll mozilla::extensions::StreamFilterParent::Init toolkit/components/extensions/webrequest/StreamFilterParent.cpp:160
1 xul.dll static mozilla::extensions::StreamFilterParent::Attach toolkit/components/extensions/webrequest/StreamFilterParent.cpp:146
2 xul.dll std::_Func_impl_no_alloc<`lambda at /builds/worker/checkouts/gecko/docshell/base/nsDocShell.cpp:12374:20', nsresult, nsDocShellLoadState*, nsTArray<mozilla::ipc::Endpoint<mozilla::extensions::PStreamFilterParent> >&&, nsDOMNavigationTiming*>::_Do_call 
3 xul.dll mozilla::dom::ChildProcessChannelListener::OnChannelReady docshell/base/ChildProcessChannelListener.cpp:33
4 xul.dll mozilla::dom::ContentChild::RecvCrossProcessRedirect dom/ipc/ContentChild.cpp:3490
5 xul.dll mozilla::dom::PContentChild::OnMessageReceived ipc/ipdl/PContentChild.cpp:12721
6 xul.dll mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2074
7 xul.dll mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1953
8 xul.dll mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:512
9 xul.dll mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:83:7'>::Run xpcom/threads/nsThreadUtils.h:577

Searchfox links for top of stack: note there is more than one trigger, see below

0. StreamFilterParent::Init @ toolkit/components/extensions/webrequest/StreamFilterParent.cpp#160
1. StreamFilterParent::Attach @ toolkit/components/extensions/webrequest/StreamFilterParent.cpp#146
2. lambda for ChildProcessChannelListener::RegisterCallback in nsDocShell::ResumeRedirectedLoad @ docshell/base/nsDocShell.cpp#12865-12866
   • To save you from loading a file with 10k+ lines, the channel parameter is aLoadState->GetPendingRedirectedChannel() - nsDocShellLoadState::GetPendingRedirectedChannel

• NOTE: This is not the only responsible caller. E.g. in bp-f30652dc-c1cc-4d82-9155-42d4f0201120 Attach was called by DocumentChannelChild::OnRedirectVerifyCallback @ netwerk/ipc/DocumentChannelChild.cpp#370

3. ChildProcessChannelListener::OnChannelReady @ docshell/base/ChildProcessChannelListener.cpp#33-34.
4. ContentChild::RecvCrossProcessRedirect @ dom/ipc/ContentChild.cpp#3509

Comment 1

[Rob Wu [:robwu]]

Do you know why we are being passed non-nsITraceableChannel instances here?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

I think there's a good chance that this is related to view-source, as :ntim and :evilpie ran into this crash trying to load the view-source:https://www.smashingmagazine.com/feed URI with the livemarks addon enabled. (from matrix)

I'm guessing there is a bug where we will crash if an add-on tries to intercept a view-source load.

Comment 3

[Rob Wu [:robwu]]

There is definitely something going on with view-source:, judging by the comments in crash reports.

But crash reports for this signature show that the top 5 of affected versions in the past 6 months is 79.0, 77.0.1, 84.0a1, 78.0.2, 80.0.1 (ranging from 9.57% to 5.39, so no version stands out).

Extensions only started to get the ability to intercept view-source: requests in 84, in https://hg.mozilla.org/mozilla-central/rev/8fd81380d432c26a9df2ea56f9330868363dec68
In earlier versions, extensions could not observe, let alone intercept view-source:-requests. So something weird is going on here.

Comment 4

[Liam England]

I ran into this crash as well loading view-source:https://www.amazon.com/ with only uBlock Origin installed.

Comment 5

[Matt Woodrow (:mattwoodrow)]

It sort of makes sense that the channel in the content process is the nsViewSourceChannel, which doesn't implement nsITraceableChannel.

I can't reproduce this though, and we have tests for this functionality, so there must be an additional factor involved here.

Comment 6

[kernp25]

Attachment: background.zip

Comment 7

[kernp25]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)

I can't reproduce this though, and we have tests for this functionality, so there must be an additional factor involved here.

STR:

1. Start Firefox Nightly (tested on 85.0a1 (2020-11-24) (64-Bit))
2. Install the test add-on
3. Load view-source:https://www.amazon.com/

Comment 8

[Rob Wu [:robwu]]

Thanks kernp for the STR. Given the consistency and ease of reproduction, I'm going to make an investigation of the issue my priority tomorrow.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

I've sent you my early WIP patch on matrix, so clearing my ni? :-)

Thanks for looking into this!

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: test.xpi

Just a slightly improved version of the extension so pages will finish loading properly.

Comment 11

[Peter deHaan [:pdehaan]]

With copious amounts of assistance from :mixedpuppy, I managed to stumble upon https://github.com/bhollis/jsonview/blob/2255064ea9daffeb54637521d946b87fe1d62ec1/src/background.ts#L24 which uses browser.webRequest.filterResponseData. You can install it in Firefox via https://addons.mozilla.org/en-US/firefox/addon/jsonview/.

To reproduce the crash in question, you can navigate to a JSON document (https://accounts.stage.mozaws.net/__version__ for example), then right-click and select “View Page Source” from the context menu and I was able to get the crash to reproduce.

Comment 12

[Tom S [:evilpie]]

I didn't know that you were still looking for causes here. This is also triggered by our extension Livemarks: https://github.com/nt1m/livemarks/issues/308

Comment 13

[Rob Wu [:robwu]]

Attachment: Bug 1678734 - Fix crash in StreamFilter with view-source

Comment 14

[Rob Wu [:robwu]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57ff06e1f0ecf1c2af44953ce8da45f46dd758d (artifact build, shows failures as expected)
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e664fc6f92b795ddf360550a20c2009994655bce (should be green)

I included unit tests and wrote a few more bad weather scenarios (https://bugzilla.mozilla.org/show_bug.cgi?id=1683189#c3), but due to bug 1683189 those tests are non-deterministic, so I either commented them out or omitted them from the patch. Locally I verified that other than bug 1683189, the tests yielded the desired results (i.e. no crashes or unexpected results).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ec315d8b8bc
Fix crash in StreamFilter with view-source r=mixedpuppy

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8ec315d8b8bc

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:robwu, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9193966 [details]
Bug 1678734 - Fix crash in StreamFilter with view-source

Beta/Release Uplift Approval Request

• User impact if declined: Attempting to viewsource a page can result in a tab crash if any extension using a streamfilter is installed. It will affect all attempts to use viewsource.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: QA has str and has done a bunch of verification with recommended addons to see if the severity was high, it is not, but we should at least uplift into beta.

https://docs.google.com/document/d/1dz4QmFfU3g6vMeQdjHT2zs4mJ3ZYkug2dcv_r-wrqXI

• List of other uplifts needed: bug 1676956
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Well tested and a small change.
• String changes made/needed: n/a

Comment 19

[Peter Magyari (Desktop QA)]

Reproduced the issue on Firefox Nightly 85.0a1 (20201124092228)
using the LiveMarks add-on @view-source:https://www.smashingmagazine.com/

Verified the fix on Firefox Nightly 86.0a1 (20201221155804)

Comment 20

[Rob Wu [:robwu]]

Shane, can you edit your uplift comment to also include bug 1676956 ? The patch in this bug registers the test in xpcshell-common-e10s.ini, which was introduced in bug 1676956

If sheriffs don't want to uplift the test-only patch from the other bug, then they would have to add [test_ext_webRequest_viewsource_StreamFilter.js] to toolkit/components/extensions/test/xpcshell/xpcshell-remote.ini and toolkit/components/extensions/test/xpcshell/xpcshell-e10s.ini

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 9193966 [details]
Bug 1678734 - Fix crash in StreamFilter with view-source

crash fix, approved for 85.0b5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Crash volume on 84 doesn't seem crazy-high. I think this can ride the 85 train.

Comment 23

[Rob Wu [:robwu]]

This feature prevents the View Source functionality from working with add-ons that use the affected API. The relative low number of crashed is likely due to the relative rarity of users that use view-source AND have such an add-on installed.

Comment 24

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f6e34d2157bf

Comment 25

[Peter Magyari (Desktop QA)]

Verified the fix on Firefox 85.0b5 (20210105185604)

Comment 29

[joant]

Hello, just wanted to note that https://bugzilla.mozilla.org/show_bug.cgi?id=1688146 includes evidence that esr78 is affected by this bug, contrary to the unaffected tag placed on this issue.

A backport to ESR would be much appreciated.

Comment 30

[Tom S [:evilpie]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/aa4b9610162034d8e4ce6ac9f721cfb509d2fea1 landed on ESR.

Comment 31

[joant]

TY :evilpie !

Comment 32

[Rob Wu [:robwu]]

Comment on attachment 9193966 [details]
Bug 1678734 - Fix crash in StreamFilter with view-source

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Causes crash, regressed by patch that has been uplifted on ESR (https://hg.mozilla.org/releases/mozilla-esr78/rev/aa4b9610162034d8e4ce6ac9f721cfb509d2fea1).
• User impact if declined: Attempting to viewsource a page can result in a tab crash if any extension using a streamfilter is installed. It will affect all attempts to use viewsource.
• Fix Landed on Version: 85
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change, covered by unit tests.
• String or UUID changes made by this patch:

Comment 33

[Julien Cristau [:jcristau]]

Comment on attachment 9193966 [details]
Bug 1678734 - Fix crash in StreamFilter with view-source

approved for 78.8esr

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Rob, can you please confirm which xpcshell manifest this test should go into on esr78? xpcshell-common-e10s.ini doesn't exist there.

Comment 35

[Rob Wu [:robwu]]

See comment 20

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Missed that note, thanks. Bug 1676956 doesn't graft cleanly so I'll opt for the latter option.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/432f7db58ff0

Comment 38

[Noemi Erli[:noemi_erli]]

Backed out changeset 432f7db58ff0 (Bug 1678734) for causing build bustages in StreamFilterParent.cpp a=backout

Push with failures: https://treeherder.mozilla.org/jobs?repo=mozilla-esr78&group_state=expanded&collapsedPushes=842382&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=f1c3dd3d8bc28a01bd388e6a9aaa3ade570d22d9&selectedTaskRun=XqjOVr8fSFWfAhABFEf5tg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=328934523&repo=mozilla-esr78&lineNumber=42971

Backout: https://hg.mozilla.org/releases/mozilla-esr78/rev/dccb143cef9b0dd07dd97c06cb52733e01ecc0ce

Comment 39

[Rob Wu [:robwu]]

Nika, do you approve of applying the following patch on top of https://hg.mozilla.org/releases/mozilla-esr78/rev/432f7db58ff0 for ESR 78?
This diff is extracted from https://hg.mozilla.org/mozilla-central/rev/51e88e67cb6e3b5f592a34f33052e5b2155b0d8a because my bugfix relies on the GetInnerChannel method (which existed before, got removed, and then restored again).

I have already verified that the build is successful, and the unit test (./mach test toolkit/components/extensions/test/xpcshell/test_ext_webRequest_viewsource_StreamFilter.js) passes with it.

diff --git a/netwerk/protocol/viewsource/nsIViewSourceChannel.idl b/netwerk/protocol/viewsource/nsIViewSourceChannel.idl
index d2703e04a2cde..4d31a427bf3c6 100644
--- a/netwerk/protocol/viewsource/nsIViewSourceChannel.idl
+++ b/netwerk/protocol/viewsource/nsIViewSourceChannel.idl
@@ -34,6 +34,11 @@ interface nsIViewSourceChannel : nsIChannel
      */
     [must_use] attribute nsIURI baseURI;

+    /**
+     * Get the inner channel wrapped by this nsIViewSourceChannel.
+     */
+    [notxpcom, nostdcall] nsIChannel getInnerChannel();
+
     /**
      * If true (default), then replaces the nsIRequest* passed to
      * nsIStreamListener callback functions with itself, so that
diff --git a/netwerk/protocol/viewsource/nsViewSourceChannel.cpp b/netwerk/protocol/viewsource/nsViewSourceChannel.cpp
index c7af6db48f5e2..afe556c9dfa6d 100644
--- a/netwerk/protocol/viewsource/nsViewSourceChannel.cpp
+++ b/netwerk/protocol/viewsource/nsViewSourceChannel.cpp
@@ -648,6 +648,8 @@ nsViewSourceChannel::SetBaseURI(nsIURI* aBaseURI) {
   return NS_OK;
 }

+nsIChannel* nsViewSourceChannel::GetInnerChannel() { return mChannel; }
+
 NS_IMETHODIMP
 nsViewSourceChannel::GetProtocolVersion(nsACString& aProtocolVersion) {
   return NS_ERROR_NOT_IMPLEMENTED;

Comment 40

[Nika Layzell [:nika] (ni? for response)]

That seems reasonable to me! Thanks for the context on matrix :-)

Comment 41

[Rob Wu [:robwu]]

Ryan, can you uplift https://hg.mozilla.org/releases/mozilla-esr78/rev/432f7db58ff0 combined with the diff from comment 39 to ESR?

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/2b4f1f232bac