Closed Bug 1597159 Opened 5 years ago Closed 5 years ago

StreamFilter won't work with Fission if we switch process

Tracking

(Fission Milestone:M5, firefox77 fixed)

Status:

RESOLVED FIXED

Milestone:

mozilla77

Project Flags:

Fission Milestone

Tracking Flags:

Tracking

Status

firefox77

---

fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1597159 - Promisify CreateStreamFilter. r?kmag 5 years ago Matt Woodrow (:mattwoodrow) 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1597159 - Defer creating DocumentLoadListener's stream filter until after we process switch. r?kmag 5 years ago Matt Woodrow (:mattwoodrow) 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1597159 - Make sure StreamFilterParent emits an OnStopRequest if we fail to forward it to the child. r?kmag 5 years ago Matt Woodrow (:mattwoodrow) 47 bytes, text/x-phabricator-request		Details \| Review

Matt Woodrow (:mattwoodrow)

Assignee

Description

•

5 years ago

nsHttpChannel::AttachStreamFilter sets up a StreamFilter actor, adding a StreamFilterParent listener in the current content process.

With fission enabled, we'll frequently do a process switch when the parent-side receives OnStartRequest.

Since we've already created the StreamFilter actors at this point, then we'll deliver an error value to the StreamFilterParent in the old process, and deliver the data to the new process (which won't have a StreamFilter attached).

Bug 1590898 broke this recently, by accidentally making it so we would attach the StreamFilter to the parent-process side of the channel. This behaviour would be fission-compatible, but since we only apply gzip decompression in the content process, it was delivering compressed data to the listener.

Some options:

Delay creating the PStreamFilter until the final content process has been selected.
Always attach to the parent-side, enable decompression in this case, maybe try to move the work off the main thread.
Recreate a new PStreamFilter actor for the new process

Matt Woodrow (:mattwoodrow)

Assignee

Updated

•

5 years ago

Summary: StreamFilter won't work with fission is we switch process → StreamFilter won't work with fission if we switch process

Kris Maglione [:kmag]

Comment 1

•

5 years ago

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

Always attach to the parent-side, enable decompression in this case, maybe try to move the work off the main thread.

As much as I'd like to do this (it was my first approach), it is apparently super nontrivial to figure out the cases where we actually want to decode the content stream from the parent process. Maybe it will be easier down the road with DocumentChannel and in-parent download handling, but I don't think we're at that point yet.

Matt Woodrow (:mattwoodrow)

Assignee

Comment 2

•

5 years ago

Yeah indeed, I think we have to handle that case for in-parent downloading with DocumentChannel, so it's possible we can solve this at the same time.

Jim Mathies [:jimm]

Updated

•

5 years ago

Priority: -- → P3

Chris Peterson [:cpeterson]

Comment 3

•

5 years ago

Tracking for Fission dogfooding (M5) because uBlock Origin depends on StreamFilter.

Fission Milestone: ? → M5

Summary: StreamFilter won't work with fission if we switch process → StreamFilter won't work with Fission if we switch process

David Teller [:Yoric] - still alive but not very active

Comment 4

•

5 years ago

So, is there anything actionable at this state by someone like me who hasn't had to deal with channels in quite a few years?

Flags: needinfo?(matt.woodrow)

Matt Woodrow (:mattwoodrow)

Assignee

Comment 5

•

5 years ago

I think we need to come to a conclusion on what the plan is (which might be possible now, given that the aforementioned in-parent download handling is done), and then we can see what work is required.

Flags: needinfo?(matt.woodrow)

Chris Peterson [:cpeterson]

Comment 6

•

5 years ago

Nika recommends we keep this bug in M5 because uBlock Origin is a very popular extension.

We might be able to create the StreamFilter after process selection in the short term and then move to the parent process longer term.

Matt Woodrow (:mattwoodrow)

Assignee

Updated

•

5 years ago

Assignee: nobody → matt.woodrow

Matt Woodrow (:mattwoodrow)

Assignee

Comment 7

•

5 years ago

WIP that defers StreamFilter creation until after process selection: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6d4e26649139c3b1109e7aeaa587abce9c43d6a

I realised in the process that doing filtering in the content process won't be catching responses that get handled entirely in the parent (like downloads), and I'm not sure if that matters.

If it does, then we'll just have to work on filtering in the parent ASAP.

Matt Woodrow (:mattwoodrow)

Assignee

Comment 8

•

5 years ago

Attached file Bug 1597159 - Promisify CreateStreamFilter. r?kmag — Details

Matt Woodrow (:mattwoodrow)

Assignee

Comment 9

•

5 years ago

Attached file Bug 1597159 - Defer creating DocumentLoadListener's stream filter until after we process switch. r?kmag — Details

Depends on D64697

Matt Woodrow (:mattwoodrow)

Assignee

Updated

•

5 years ago

Blocks: 1623921

Shane Caraveo (:mixedpuppy)

Comment 10

•

5 years ago

I'd like to figure out how to get these patches moving forward.

Priority: P3 → P1

Shane Caraveo (:mixedpuppy)

Updated

•

5 years ago

Blocks: 1626826

Matt Woodrow (:mattwoodrow)

Assignee

Comment 11

•

5 years ago

Attached file Bug 1597159 - Make sure StreamFilterParent emits an OnStopRequest if we fail to forward it to the child. r?kmag — Details

Depends on D64698

Pulsebot

Comment 12

•

5 years ago

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/927e82a323c6 Promisify CreateStreamFilter. r=kmag https://hg.mozilla.org/integration/autoland/rev/0ad16f200740 Defer creating DocumentLoadListener's stream filter until after we process switch. r=kmag https://hg.mozilla.org/integration/autoland/rev/2dd2598b3edb Make sure StreamFilterParent emits an OnStopRequest if we fail to forward it to the child. r=kmag

Cristian Brindusan [:cbrindusan]

Comment 13

•

5 years ago

Backed out 3 changesets (Bug 1597159) for build bustages on netwerk/ipc/target-objects.
https://hg.mozilla.org/integration/autoland/rev/1fab775cc7c0a2c6b5c5a52bf3148449965d528f

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=296894286&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=2dd2598b3edbe623eb91e15a0280223c74d82470

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296894286&repo=autoland&lineNumber=21518

Flags: needinfo?(matt.woodrow)

Bogdan Tara[:bogdan_tara | bogdant]

Comment 14

•

5 years ago

Also seeing these failures on the backed out changes:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2C18.04%2Cx64%2Cshippable%2Copt%2Cmochi&fromchange=2dd2598b3edbe623eb91e15a0280223c74d82470&tochange=1fab775cc7c0a2c6b5c5a52bf3148449965d528f&selectedJob=296904117

https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2C18.04%2Cx64%2Cshippable%2Copt%2Cmochi&fromchange=2dd2598b3edbe623eb91e15a0280223c74d82470&tochange=1fab775cc7c0a2c6b5c5a52bf3148449965d528f&selectedJob=296900897

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296904117&repo=autoland&lineNumber=9322

[task 2020-04-09T06:45:05.860Z] 06:45:05     INFO - TEST-START | toolkit/components/extensions/test/mochitest/test_ext_streamfilter_multiple.html
[task 2020-04-09T06:45:06.185Z] 06:45:06     INFO - GECKO(11861) | ###!!! [Child][RunMessage] Error: Channel closing: too late to send/recv, messages will be lost
[task 2020-04-09T06:45:06.229Z] 06:45:06     INFO - TEST-INFO | started process screentopng
[task 2020-04-09T06:45:07.104Z] 06:45:07     INFO - TEST-INFO | screentopng: exit 0
[task 2020-04-09T06:45:07.105Z] 06:45:07     INFO - Buffered messages logged at 06:45:06
[task 2020-04-09T06:45:07.106Z] 06:45:07     INFO - add_task | Entering test 
[task 2020-04-09T06:45:07.107Z] 06:45:07     INFO - Extension loaded
[task 2020-04-09T06:45:07.108Z] 06:45:07     INFO - Extension loaded
[task 2020-04-09T06:45:07.109Z] 06:45:07     INFO - Buffered messages finished
[task 2020-04-09T06:45:07.110Z] 06:45:07     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_streamfilter_multiple.html | Correctly intercepted page content - got "Start Middle\n", expected "Start Middle\n End"
[task 2020-04-09T06:45:07.111Z] 06:45:07     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:383:14
[task 2020-04-09T06:45:07.112Z] 06:45:07     INFO -     @toolkit/components/extensions/test/mochitest/test_ext_streamfilter_multiple.html:82:14
[task 2020-04-09T06:45:07.112Z] 06:45:07     INFO -     async*nextTick/<@SimpleTest/SimpleTest.js:2090:34
[task 2020-04-09T06:45:07.113Z] 06:45:07     INFO -     nextTick@SimpleTest/SimpleTest.js:2115:11
[task 2020-04-09T06:45:07.114Z] 06:45:07     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:788:41
[task 2020-04-09T06:45:07.115Z] 06:45:07     INFO -     add_task@SimpleTest/SimpleTest.js:2045:17
[task 2020-04-09T06:45:07.116Z] 06:45:07     INFO -     @toolkit/components/extensions/test/mochitest/test_ext_streamfilter_multiple.html:18:9
[task 2020-04-09T06:45:07.117Z] 06:45:07     INFO - add_task | Leaving test 
[task 2020-04-09T06:45:07.117Z] 06:45:07     INFO - GECKO(11861) | MEMORY STAT | vsize 2524MB | residentFast 133MB | heapAllocated 15MB
[task 2020-04-09T06:45:07.118Z] 06:45:07     INFO - TEST-OK | toolkit/components/extensions/test/mochitest/test_ext_streamfilter_multiple.html | took 417ms

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296900897&repo=autoland&lineNumber=12811

task 2020-04-09T06:13:21.799Z] 06:13:21     INFO - TEST-START | dom/tests/mochitest/whatwg/test_postMessage_onOther.html
[task 2020-04-09T06:13:22.561Z] 06:13:22     INFO - GECKO(13704) | JavaScript error: http://example.com/tests/dom/tests/mochitest/whatwg/postMessage_onOther.html, line 88: SecurityError: Permission denied to access property "testSiblingPostMessage" on cross-origin object
[task 2020-04-09T06:18:46.925Z] 06:18:46     INFO - TEST-INFO | started process screentopng
[task 2020-04-09T06:18:47.077Z] 06:18:47     INFO - TEST-INFO | screentopng: exit 0
[task 2020-04-09T06:18:47.078Z] 06:18:47     INFO - TEST-UNEXPECTED-FAIL | dom/tests/mochitest/whatwg/test_postMessage_onOther.html | Test timed out. 
[task 2020-04-09T06:18:47.078Z] 06:18:47     INFO -     SimpleTest.ok@SimpleTest/SimpleTest.js:299:16
[task 2020-04-09T06:18:47.078Z] 06:18:47     INFO -     reportError@SimpleTest/TestRunner.js:128:22
[task 2020-04-09T06:18:47.078Z] 06:18:47     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:150:18
[task 2020-04-09T06:18:47.078Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.078Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.079Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.079Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.079Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.079Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.079Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.080Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.080Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.080Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.080Z] 06:18:47     INFO -     setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:184:15
[task 2020-04-09T06:18:47.081Z] 06:18:47     INFO -     TestRunner.runTests/<@SimpleTest/TestRunner.js:420:16
[task 2020-04-09T06:18:47.081Z] 06:18:47     INFO -     promise callback*TestRunner.runTests@SimpleTest/TestRunner.js:407:48
[task 2020-04-09T06:18:47.082Z] 06:18:47     INFO -     RunSet.runtests@SimpleTest/setup.js:218:14
[task 2020-04-09T06:18:47.082Z] 06:18:47     INFO -     RunSet.runall@SimpleTest/setup.js:197:12
[task 2020-04-09T06:18:47.082Z] 06:18:47     INFO -     hookupTests@SimpleTest/setup.js:294:12
[task 2020-04-09T06:18:47.082Z] 06:18:47     INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:48:13
[task 2020-04-09T06:18:47.083Z] 06:18:47     INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:61:28
[task 2020-04-09T06:18:47.083Z] 06:18:47     INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:57:3
[task 2020-04-09T06:18:47.083Z] 06:18:47     INFO -     hookup@SimpleTest/setup.js:270:20
[task 2020-04-09T06:18:47.084Z] 06:18:47     INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp&cleanupCrashes=true:11:1
[task 2020-04-09T06:18:47.927Z] 06:18:47     INFO - GECKO(13704) | MEMORY STAT | vsize 2498MB | residentFast 102MB | heapAllocated 10MB
[task 2020-04-09T06:18:47.929Z] 06:18:47     INFO - TEST-OK | dom/tests/mochitest/whatwg/test_postMessage_onOther.html | took 326136ms

Pulsebot

Comment 15

•

5 years ago

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/037b03cc5009 Promisify CreateStreamFilter. r=kmag https://hg.mozilla.org/integration/autoland/rev/85543665929e Defer creating DocumentLoadListener's stream filter until after we process switch. r=kmag https://hg.mozilla.org/integration/autoland/rev/ef27657560d1 Make sure StreamFilterParent emits an OnStopRequest if we fail to forward it to the child. r=kmag

Cosmin Sabou [:CosminS]

Comment 16

•

5 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/037b03cc5009
https://hg.mozilla.org/mozilla-central/rev/85543665929e
https://hg.mozilla.org/mozilla-central/rev/ef27657560d1

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox77: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla77

Matt Woodrow (:mattwoodrow)

Assignee

Updated

•

5 years ago

Flags: needinfo?(matt.woodrow)

Bogdan Tara[:bogdan_tara | bogdant]

Updated

•

5 years ago

Regressions: 1628642

Johannes Pfrang [:johnp]

Updated

•

5 years ago

Regressions: 1628766

You need to log in before you can comment on or make changes to this bug.