Crash in [@ mozilla::extensions::StreamFilterParent::Init] MOZ_RELEASE_ASSERT(traceable)
Categories
(WebExtensions :: Request Handling, defect, P1)
Tracking
(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)
756 bytes,
application/x-xpinstall
|
Details | |
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
StreamFilterParent::Init
@ toolkit/components/extensions/webrequest/StreamFilterParent.cpp#160StreamFilterParent::Attach
@ toolkit/components/extensions/webrequest/StreamFilterParent.cpp#146- lambda for
ChildProcessChannelListener::RegisterCallback
innsDocShell::ResumeRedirectedLoad
@ docshell/base/nsDocShell.cpp#12865-12866- To save you from loading a file with 10k+ lines, the
channel
parameter isaLoadState->GetPendingRedirectedChannel()
-nsDocShellLoadState::GetPendingRedirectedChannel
- To save you from loading a file with 10k+ lines, the
- NOTE: This is not the only responsible caller. E.g. in bp-f30652dc-c1cc-4d82-9155-42d4f0201120
Attach
was called byDocumentChannelChild::OnRedirectVerifyCallback
@ netwerk/ipc/DocumentChannelChild.cpp#370
ChildProcessChannelListener::OnChannelReady
@ docshell/base/ChildProcessChannelListener.cpp#33-34.ContentChild::RecvCrossProcessRedirect
@ dom/ipc/ContentChild.cpp#3509
Assignee | ||
Comment 1•3 years ago
|
||
Do you know why we are being passed non-nsITraceableChannel
instances here?
Comment 2•3 years ago
|
||
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.
Assignee | ||
Comment 3•3 years ago
|
||
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.
Comment 5•3 years ago
|
||
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.
(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:
- Start Firefox Nightly (tested on 85.0a1 (2020-11-24) (64-Bit))
- Install the test add-on
- Load
view-source:https://www.amazon.com/
Assignee | ||
Comment 8•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 9•3 years ago
|
||
I've sent you my early WIP patch on matrix, so clearing my ni? :-)
Thanks for looking into this!
Comment 10•3 years ago
|
||
Just a slightly improved version of the extension so pages will finish loading properly.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 11•3 years ago
|
||
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•3 years ago
|
||
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
Assignee | ||
Comment 13•3 years ago
|
||
Assignee | ||
Comment 14•3 years ago
•
|
||
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•3 years ago
|
||
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/8ec315d8b8bc Fix crash in StreamFilter with view-source r=mixedpuppy
Comment 16•3 years ago
|
||
bugherder |
Comment 17•3 years ago
|
||
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•3 years ago
•
|
||
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
Updated•3 years ago
|
Updated•3 years ago
|
Comment 19•3 years ago
•
|
||
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)
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 20•3 years ago
|
||
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•3 years ago
|
||
Comment on attachment 9193966 [details]
Bug 1678734 - Fix crash in StreamFilter with view-source
crash fix, approved for 85.0b5
Comment 22•3 years ago
|
||
Crash volume on 84 doesn't seem crazy-high. I think this can ride the 85 train.
Assignee | ||
Comment 23•3 years ago
|
||
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•3 years ago
|
||
bugherder uplift |
Comment 25•3 years ago
|
||
Verified the fix on Firefox 85.0b5 (20210105185604)
Comment 29•3 years ago
|
||
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•3 years ago
|
||
Updated•3 years ago
|
Comment 31•3 years ago
|
||
TY :evilpie !
Assignee | ||
Comment 32•3 years ago
•
|
||
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•3 years ago
|
||
Comment on attachment 9193966 [details]
Bug 1678734 - Fix crash in StreamFilter with view-source
approved for 78.8esr
Comment 34•3 years ago
|
||
Rob, can you please confirm which xpcshell manifest this test should go into on esr78? xpcshell-common-e10s.ini doesn't exist there.
Comment 36•3 years ago
|
||
Missed that note, thanks. Bug 1676956 doesn't graft cleanly so I'll opt for the latter option.
Comment 37•3 years ago
|
||
bugherder uplift |
Comment 38•3 years ago
|
||
Backed out changeset 432f7db58ff0 (Bug 1678734) for causing build bustages in StreamFilterParent.cpp a=backout
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
Assignee | ||
Comment 39•3 years ago
|
||
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•3 years ago
|
||
That seems reasonable to me! Thanks for the context on matrix :-)
Assignee | ||
Comment 41•3 years ago
|
||
Ryan, can you uplift https://hg.mozilla.org/releases/mozilla-esr78/rev/432f7db58ff0 combined with the diff from comment 39 to ESR?
Updated•3 years ago
|
Comment 42•3 years ago
|
||
bugherder uplift |
Updated•3 years ago
|
Updated•3 years ago
|
Description
•