Closed Bug 1678734 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Request Handling, defect, P1)

Product:
WebExtensions
Component:
Request Handling
Platform:
Unspecified
All
Type:
defect

Tracking

(firefox-esr78 fixed, firefox83 wontfix, firefox84- wontfix, firefox85+ verified, firefox86+ verified)

Status:
VERIFIED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox-esr78 --- fixed
firefox83 --- wontfix
firefox84 - wontfix
firefox85 + verified
firefox86 + verified

People

(Reporter: robwu, Assigned: robwu)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files, 1 obsolete file)

test.xpi
756 bytes, application/x-xpinstall
Details
Bug 1678734 - Fix crash in StreamFilter with view-source
48 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
jcristau
: approval-mozilla-esr78+
Details | Review

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

  1. StreamFilterParent::Init @ toolkit/components/extensions/webrequest/StreamFilterParent.cpp#160
  2. StreamFilterParent::Attach @ toolkit/components/extensions/webrequest/StreamFilterParent.cpp#146
  3. lambda for ChildProcessChannelListener::RegisterCallback in nsDocShell::ResumeRedirectedLoad @ docshell/base/nsDocShell.cpp#12865-12866
  1. ChildProcessChannelListener::OnChannelReady @ docshell/base/ChildProcessChannelListener.cpp#33-34.
  2. ContentChild::RecvCrossProcessRedirect @ dom/ipc/ContentChild.cpp#3509

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

Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(nika)

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.

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

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.

Flags: needinfo?(matt.woodrow)
Attached file background.zip (obsolete) —

(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/
Flags: needinfo?(matt.woodrow)

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.

Assignee: nobody → rob
Status: NEW → ASSIGNED
Flags: needinfo?(matt.woodrow)

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

Thanks for looking into this!

Flags: needinfo?(nika)
Attached file test.xpi —

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

Attachment #9193287 - Attachment is obsolete: true
Severity: -- → S2
Priority: -- → P1

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.

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

See Also: → 1683189

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).

See Also: → 1683403
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/8ec315d8b8bc
Fix crash in StreamFilter with view-source r=mixedpuppy

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

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox86: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

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.

Flags: needinfo?(rob)

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
Attachment #9193966 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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)

status-firefox86: fixed → verified
Flags: needinfo?(rob)

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

Depends on: 1676956

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

crash fix, approved for 85.0b5

Attachment #9193966 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

status-firefox84: affected → wontfix
tracking-firefox84: + → -

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.

Verified the fix on Firefox 85.0b5 (20210105185604)

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
status-firefox85: fixed → verified
Flags: qe-verify+

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.

TY :evilpie !

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:
Attachment #9193966 - Flags: approval-mozilla-esr78?

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

approved for 78.8esr

Attachment #9193966 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

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

Flags: needinfo?(rob)

See comment 20

Flags: needinfo?(rob)

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

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;
Flags: needinfo?(rob) → needinfo?(nika)

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

Flags: needinfo?(nika)

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

Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Keywords: regression
Regressed by: CVE-2020-35111
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.