1641737 - Have ADocumentChannelBridge <-> DocumentLoadListener communicate via MozPromise

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P3)

Description

[Jean-Yves Avenard [:jya]]

This is a change we discussed with :mattwoodrow today and will play very nicely in simplifying the communication between a DocumentChannel and a DocumentLoadListener.

Right now, we have a cycle between the DocumentChannel (DC) and a DocumentLoadListener (DLL) ; the DC sends info to the DLL such as when to start (via Open) ; when to finish or disconnect.

The DLL on the other hand, needs to also know when to disconnect, and when the redirection has been completed.

The cycle in place between the two has created a fair amount of complexity, some UAF error ; and we have issues where the DC would resolve the RedirectToRealChannel promise , and then disconnect the DLL ; but due to MozPromise implementation the task telling about the resolution gets run after the DLL has actually been disconnected.

So the idea is for the DocumentChannel to call DLL::Open which would return a promise ; this promise would be resolved/rejected once the DLL is done or when it needs for the redirect to the real channel started.

The DLL would no longer hold a reference to the ADocumentChannelBridge ; removing the cycle and simplifying on what to do when we're done or there's an error.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1641737 - P1. Set dispatch type to chained promises. r=bholley

When chaining a MozPromise set to be dispatched via the direct task queue (or synchronous), it makes sense for the chained promise to be dispatched in the same fashion.

All MozPromises generated by the IPC bindings are set to use the direct task queue in order to prevent the then runnable to run out of order with other IPC tasks.
We want to preserve that task ordering by default.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1641737 - P2. Use MozPromise with DocumentLoadListener and remove cycle. r=mattwoodrow.

Depends on D78178

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1641737 - P3. Streamline cleanup so it's all done in the same place. r=mattwoodrow

Depends on D78179

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1641737 - P4. Add UseDirectTaskDispatch/UseSynchronousDispatch to MozPromiseHolder. r=bholley

The allows to not have to deal with MozPromise::Private directly.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1641737 - P5. Remove unnecessary method. r=mattwoodrow

We would have ended up attempting to tell the child that it just died.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1641737 - P6. Remove the need for an extra promise for when a DocumentChannel would connect. r=mattwoodrow

The earlier changes make this unnecessary. The load will start and will automatically resume once the PDocumentChannelParent::RedirectToRealChannel promise gets resolved when a DocumentChannel claim the DocumentLoadListener.

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1641737 - P7. Make the DLL and PPDC promises use direct task dispatch. r=mattwoodrow

We attempt to reduce the number of event loop iterations, which would bring us back to a similar behaviour to the pre-DocumentChannel days.

This prevent some tests to have increase intermittent failures.

While ideally, we would prefer to fix or re-write those tests properly, the extra work would block us for too long.

All those increased intermittents already have bug logged about them; so we aren't hiding dust under the rust.

Depends on D78488

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0296d0f6c1d1
P1. Set dispatch type to chained promises. r=bholley
https://hg.mozilla.org/integration/autoland/rev/28bca5c5b8b4
P2. Use MozPromise with DocumentLoadListener and remove cycle. r=mattwoodrow.
https://hg.mozilla.org/integration/autoland/rev/94d47578009c
P3. Streamline cleanup so it's all done in the same place. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/e3bf9a45db6a
P4. Add UseDirectTaskDispatch/UseSynchronousDispatch to MozPromiseHolder. r=bholley
https://hg.mozilla.org/integration/autoland/rev/abd9cd77f3cb
P5. Remove unnecessary method. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/32fba417ebd0
P6. Remove the need for an extra promise for when a DocumentChannel would connect. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/7f7b98339065
P7. Make the DLL and PPDC promises use direct task dispatch. r=mattwoodrow

Comment 9

[Razvan Maries]

Backed out for perma failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Cwithout%2Ce10s%2Ctest-macosx1014-64%2Fdebug-mochitest-chrome-1proc-2%2Cm-1proc%28c2%29&revision=7f7b983390650cbc7d736e92fd3e1f629a30ac02&selectedTaskRun=Rq-UrJe1QnakASK2Qs_5GQ-0

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

Backout: https://hg.mozilla.org/integration/autoland/rev/0f4b97c32ba05a9e4285a2c32add44305c2aa727

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/0296d0f6c1d1
https://hg.mozilla.org/mozilla-central/rev/28bca5c5b8b4
https://hg.mozilla.org/mozilla-central/rev/94d47578009c
https://hg.mozilla.org/mozilla-central/rev/e3bf9a45db6a
https://hg.mozilla.org/mozilla-central/rev/abd9cd77f3cb
https://hg.mozilla.org/mozilla-central/rev/32fba417ebd0
https://hg.mozilla.org/mozilla-central/rev/7f7b98339065

Comment 11

[Jean-Yves Avenard [:jya]]

so we are now blocked on bug 1644009, which will remove the dependency on AbstractThread to use direct task dispatching.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02f45f660107
P1. Set dispatch type to chained promises. r=bholley
https://hg.mozilla.org/integration/autoland/rev/81ddcb21f057
P2. Use MozPromise with DocumentLoadListener and remove cycle. r=mattwoodrow.
https://hg.mozilla.org/integration/autoland/rev/f1364c78caa7
P3. Streamline cleanup so it's all done in the same place. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/f6c685e4b841
P4. Add UseDirectTaskDispatch/UseSynchronousDispatch to MozPromiseHolder. r=bholley
https://hg.mozilla.org/integration/autoland/rev/3998aeb0935c
P5. Remove unnecessary method. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/bcbaea8b572e
P6. Remove the need for an extra promise for when a DocumentChannel would connect. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/3922996e51fa
P7. Make the DLL and PPDC promises use direct task dispatch. r=mattwoodrow

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/02f45f660107
https://hg.mozilla.org/mozilla-central/rev/81ddcb21f057
https://hg.mozilla.org/mozilla-central/rev/f1364c78caa7
https://hg.mozilla.org/mozilla-central/rev/f6c685e4b841
https://hg.mozilla.org/mozilla-central/rev/3998aeb0935c
https://hg.mozilla.org/mozilla-central/rev/bcbaea8b572e
https://hg.mozilla.org/mozilla-central/rev/3922996e51fa