1264513 - [Presentation WebAPI] Enable OOP RTCDataChannel session transport

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Junior [inactive]]

We need to enable the oop part.

Comment 1

[Junior [inactive]]

Attachment: use data channel in substitution for TCP session transport (oop) WIP v1

Comment 2

[Junior [inactive]]

Attachment: use data channel in substitution for TCP session transport (oop) WIP v2

Since bug 1257734 seems not going to land, I change the sender test to use loadPrivilegedScript (bug 1264513)

Comment 3

[Junior [inactive]]

Attachment: use data channel in substitution for TCP session transport (oop) WIP v3

loadFrameScript enables the receiver_oop with some bustage

Comment 4

[Junior [inactive]]

Attachment: use data channel in substitution for TCP session transport (oop) WIP v4

Finish receiver oop test.
TODO: test controlchannel IPC

Comment 5

[Junior [inactive]]

Attachment: use data channel in substitution for TCP session transport (oop) WIP v5

test IPC
TODO: enrich test coverage and error handling

Comment 6

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bef0d858490

Comment 7

[Junior [inactive]]

Attachment: use data channel in substitution for TCP session transport (oop) v6

Known issue is leakage. But I'd like to ask for review at this time. And I'll continue to cook the document to ease the review effort.

Comment 8

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25e7366922a0

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

uh, that is quite some large patch ;)
It might be worth to split tests to a separate patch

Comment 10

[Junior [inactive]]

Comment on attachment 8745286 [details] [diff] [review]
use data channel in substitution for TCP session transport (oop) v6

Cancel the review request since we need to split the patch.
And I'm concentrate in a weird crash.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Sorry that I was slow with the review.

Once you upload a new patch, could you hint how urgent it is so that I could prioritize reviewing it if needed.

Comment 12

[Junior [inactive]]

Have solve the crash issue and leakage.
But hit the assertion
Assertion failure: typeDescrObjects.empty(), at js/src/gc/Zone.cpp:65

Comment 13

[Junior [inactive]]

Attachment: Part 1: PresentationParent as a PresentationDataChannelSessionTransportBuilder, v1

This patch lets
1) PresentationParent implement PresentationDataChannelSessionTransportBuilder 
2) PPresentationControlChannel be a proxy of the underlying control channel between two processes
3) PresentationControlChannelChild use DataChannelBuilder to build the transport in the content process. Notify the result to the parent process.

Comment 14

[Junior [inactive]]

Attachment: Part 2: Manage transport in content process, v1

Let IPCService handle the OOP session transport

Comment 15

[Junior [inactive]]

Attachment: Part 3: Hook session transport builder, v1

Session info will start to use PresentationParent as a data channel builder.

Comment 16

[Junior [inactive]]

Attachment: Part4: test, v1

Comment 17

[Junior [inactive]]

Note that we still hit the assertion mentioned in Comment 12.

Comment 18

[Junior [inactive]]

Attachment: Part 4: test, v2

fix a nit

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Reviews are coming. Sorry about delay. Trying to get to this and other presentation API patches tomorrow.

Comment 20

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d754623f75b0

Comment 21

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8676e51e40b9

Comment 22

[Junior [inactive]]

Attachment: Part 1: PresentationParent as a PresentationDataChannelSessionTransportBuilder, v2

Comment 23

[Junior [inactive]]

Attachment: Part 4: test, v3

Solve the leakage in the content process which is hidden by assertion.

Per discussion with the author of assertion, the root cause of the assertion maybe from a leakage.

Let's check in the treeherder

Comment 24

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdefabd13a7e

Comment 25

[Junior [inactive]]

Attachment: Part 1: PresentationParent as a PresentationDataChannelSessionTransportBuilder, v3

Remove the wrong modification.

And it's confirmed that the assertion in Comment 12 is solved by removing the leakage.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Perhaps schien could give feedback before a review.

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Olli Pettay [:smaug] from comment #26)
> Perhaps schien could give feedback before a review.

I plan to do this today.

Comment 28

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8752159 [details] [diff] [review]
Part 1: PresentationParent as a PresentationDataChannelSessionTransportBuilder, v3

Review of attachment 8752159 [details] [diff] [review]:
-----------------------------------------------------------------

The IPC boundary (nsIPresentationService and nsIPresentationControlChannel) you chose makes your code hard to trace. I would suggest you put a more detailed diagram to illustrate the interaction over IPC.

::: dom/presentation/ipc/PPresentation.ipdl
@@ +58,5 @@
>    async NotifyAvailableChange(bool aAvailable);
>    async NotifySessionStateChange(nsString aSessionId, uint16_t aState);
>    async NotifyMessage(nsString aSessionId, nsCString aData);
>    async NotifySessionConnect(uint64_t aWindowId, nsString aSessionId);
> +  async NotifyTransportBuild(nsString aSessionId, uint8_t aType);

you can directly use "async PPresentationControlChannel(nsString aSessionId, uint8_t aType)" to construct the IPDL actor.

::: dom/presentation/ipc/PresentationIPCService.cpp
@@ +212,5 @@
> +  actor.forget(&sPresentationControlChannelChild); // IPDL owns this reference now.
> +
> +  if (NS_WARN_IF(NS_FAILED(sPresentationControlChannelChild->Init()))) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }

By considering the Firefox e10s architecture, there will be multiple page on the same content process. One instance of control channel is for one ongoing presentation request so using a static pointer to hold the reference seems wrong to me.

Comment 29

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: [Proposal by SC] PresentationSessionTransport.png

This is the IPC boundary I have in mind. We should be able to isolate the code by making TransportBuilder interface IPC-friendly.

Comment 30

[Junior [inactive]]

Comment on attachment 8752159 [details] [diff] [review]
Part 1: PresentationParent as a PresentationDataChannelSessionTransportBuilder, v3

Per offline discussion, we plan to do some modification as following:
1. Fix issues in comment 28

2. To avoid complicated triangle reference, maintain the control channel in PresentationSessionInfo, not passing to builder. PresentationSessionInfo would be the mediator between control channel and builder.

3. PPresentationControlChannel would be renamed to PPresentationDataChannelBuilder. Similar rename policy with those actors.

4. Instead of PresentationParent, a new class PresentationOOPDataChannelBuilder will implements Builder and handle the IPC (PPresentationDataChannelBuilder)

Comment 31

[Junior [inactive]]

Attachment: ClassDiagram.jpg

The class diagram during the transport building process

Comment 32

[Junior [inactive]]

Note that there's a crash bustage for Linux x64 opt indicated by command 24.

Comment 33

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08553df48dd7

Comment 34

[Junior [inactive]]

Attachment: Part 1: Implement OOP builder in chrome process, v1

Comment 35

[Junior [inactive]]

Attachment: Part 2: Manage transport in content process, v2

Comment 36

[Junior [inactive]]

Attachment: Part 3: Hook session transport builder, v2

Comment 37

[Junior [inactive]]

Attachment: Part 4: test, v4

Comment 38

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8754766 [details] [diff] [review]
Part 1: Implement OOP builder in chrome process, v1

Review of attachment 8754766 [details] [diff] [review]:
-----------------------------------------------------------------

one suggestion for the patch partitioning:
part 1: nsIPresentationSessionTransportBuilder.idl changes - necessary refactory in in-proc data channel handling
part 2: PPresentationBuilder.ipdl changes - OOP handling for builder
part 3: new test cases for oop data channel scenario

::: dom/presentation/DCPresentationChannelDescription.h
@@ +26,5 @@
> +
> +  nsString mSDP;
> +};
> +
> +NS_IMPL_ISUPPORTS(DCPresentationChannelDescription, nsIPresentationChannelDescription)

please put class implementation in cpp.

::: dom/presentation/PresentationOOPDataChannelBuilder.cpp
@@ +18,5 @@
> +NS_IMETHODIMP
> +PresentationOOPDataChannelBuilder::
> +  BuildDataChannelTransport(uint8_t aRole,
> +                            mozIDOMWindow* aWindow, /* unused */
> +                            nsIPresentationDataChannelSessionTransportBuilderListener* aListener)

nit: prefer this kind of line breaking rule https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsChild.cpp#1665

@@ +21,5 @@
> +                            mozIDOMWindow* aWindow, /* unused */
> +                            nsIPresentationDataChannelSessionTransportBuilderListener* aListener)
> +{
> +  RefPtr<PresentationBuilderParent> actor =
> +    new PresentationBuilderParent(aRole, aListener);

PresentationOOPDataChannelBuilder and PresentationBuilderParent can be merged into one class.

::: dom/presentation/interfaces/nsIPresentationSessionTransportBuilder.idl
@@ +24,5 @@
> +{
> +  void sendOffer(in nsIPresentationChannelDescription offer);
> +  void sendAnswer(in nsIPresentationChannelDescription answer);
> +  void sendIceCandidate(in DOMString candidate);
> +  void close(in nsresult reason);

Can we simply add those functions into nsIPresentationSessionTransportBuilerListener?

::: dom/presentation/ipc/PresentationParent.cpp
@@ +12,4 @@
>  #include "PresentationParent.h"
>  #include "PresentationService.h"
> +#include "PresentationSessionInfo.h"
> +

nit: remove extra empty line

::: dom/presentation/ipc/PresentationParent.h
@@ +14,4 @@
>  #include "nsIPresentationListener.h"
>  #include "nsIPresentationService.h"
> +#include "nsIPresentationSessionTransportBuilder.h"
> +

nit: remove the extra empty line.

@@ +52,5 @@
> +  virtual bool
> +  DeallocPPresentationBuilderParent(
> +    PPresentationBuilderParent* aActor) override;
> +
> +

nit: remove extra new line

@@ +79,2 @@
>    nsCOMPtr<nsIPresentationService> mService;
> +

nit: remove extra new line

@@ +140,5 @@
> +private:
> +  virtual ~PresentationBuilderParent();
> +  bool mActorDestroyed = false;
> +  nsCOMPtr<nsIPresentationService> mService;
> +  RefPtr<PresentationParent> mParent;

PPresentationBuilder is managed by PPresentation so you can call Manager() to get PPresentation object in builder.

Comment 39

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8754767 [details] [diff] [review]
Part 2: Manage transport in content process, v2

Review of attachment 8754767 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/ipc/PresentationIPCSessionInfo.h
@@ +16,5 @@
> +namespace dom {
> +
> +class PresentationIPCService;
> +
> +class PresentationIPCSessionInfo final: public nsIPresentationSessionTransportCallback

I'm a bit concern about the naming because this class is not doing any IPC things. This is just a hook for SessionTransport so you might want to rename it.

@@ +45,5 @@
> +
> +  nsresult Close(nsresult aReason);
> +
> +private:
> +  ~PresentationIPCSessionInfo() {}

virtual

Comment 40

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8754768 [details] [diff] [review]
Part 3: Hook session transport builder, v2

Review of attachment 8754768 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/PresentationSessionInfo.cpp
@@ +440,5 @@
> + * In this case, |mBuilder| would be an object of |PresentationParent| and set
> + * previously. Therefore, |BuildDataChannelTransport| triggers an IPC call to
> + * make content process establish a RTCDataChannel transport.
> + */
> +bool PresentationSessionInfo::IsOOPDataChannelTransport()

return value doesn't matched with the function name after OnSessionTranport / OnError. Since you only use this function in on place, you can just do an if-check in NotifyOpened.

::: dom/presentation/ipc/PresentationParent.cpp
@@ +217,5 @@
> +
> +  nsCOMPtr<nsIPresentationSessionTransportBuilder> builder =
> +    new PresentationOOPDataChannelBuilder(this);
> +  NS_WARN_IF(NS_FAILED(static_cast<PresentationService*>(mService.get())->
> +                         RegisterTransportBuilder(aSessionId, aRole, builder)));

You can do this in |PresentationRequestParent::DoRequest(const StartSessionRequest& aRequest)| and PresentationParent::RecvNotifyReceiverReady without additional IPDL interface.

Comment 41

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8754770 [details] [diff] [review]
Part 4: test, v4

Review of attachment 8754770 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/tests/mochitest/PresentationSessionChromeScript.js
@@ +200,5 @@
>    }
>  };
>  
>  const mockedSessionTransport = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIInterfaceRequestor,

You didn't use the nsIInterfaceRequest anymore, do you?

::: dom/presentation/tests/mochitest/test_presentation_dc_receiver_oop.html
@@ +170,5 @@
> +    if (gScriptTeardownComplete) {
> +      SimpleTest.finish();
> +    }
> +  });
> +  

nit: tailing spaces, here and elsewhere.

::: modules/libpref/init/all.js
@@ +5126,5 @@
>  pref("dom.presentation.tcp_server.debug", false);
>  pref("dom.presentation.discovery.enabled", false);
>  pref("dom.presentation.discovery.timeout_ms", 10000);
>  pref("dom.presentation.discoverable", false);
> +pref("dom.presentation.session_transport.data_channel.enable", true);

This change shouldn't be included in this part of patches.

Comment 42

[Junior [inactive]]

> one suggestion for the patch partitioning:
> part 1: nsIPresentationSessionTransportBuilder.idl changes - necessary
> refactory in in-proc data channel handling
> part 2: PPresentationBuilder.ipdl changes - OOP handling for builder
> part 3: new test cases for oop data channel scenario
> 
part 2 will be a large patch this way.
Do you think merging would be a good idea since it's easier to trace?
Or we can still split it to three patches like:
Part 2-1: implement PresentationBuilder
Part 2-2: manage session transport in content process
Part 2-3: hook session transport builder

> 
> @@ +21,5 @@
> > +                            mozIDOMWindow* aWindow, /* unused */
> > +                            nsIPresentationDataChannelSessionTransportBuilderListener* aListener)
> > +{
> > +  RefPtr<PresentationBuilderParent> actor =
> > +    new PresentationBuilderParent(aRole, aListener);
> 
> PresentationOOPDataChannelBuilder and PresentationBuilderParent can be
> merged into one class.
> 
> @@ +140,5 @@
> > +private:
> > +  virtual ~PresentationBuilderParent();
> > +  bool mActorDestroyed = false;
> > +  nsCOMPtr<nsIPresentationService> mService;
> > +  RefPtr<PresentationParent> mParent;
> 
> PPresentationBuilder is managed by PPresentation so you can call Manager()
> to get PPresentation object in builder.

Just a heads-up.
If we merge PresentationOOPDataChannelBuilder to PresentationBuilderParent, the underlying IPC channel will establish by |SendPPresentationBuilderConstructor| after |BuildDataChannelTransport|. After that, |Manager()| is valid to get |PPresetationParent| object.

Therefore, I still need to have |RefPtr<PresentationParent> mParent| to send the constructor.


> @@ +440,5 @@
> > + * In this case, |mBuilder| would be an object of |PresentationParent| and set
> > + * previously. Therefore, |BuildDataChannelTransport| triggers an IPC call to
> > + * make content process establish a RTCDataChannel transport.
> > + */
> > +bool PresentationSessionInfo::IsOOPDataChannelTransport()
> 
> return value doesn't matched with the function name after OnSessionTranport
> / OnError. Since you only use this function in on place, you can just do an
> if-check in NotifyOpened.
> 
Actually the function is used in two places and I need a good place to put these comment.
How about |return !!mBuilder|?

Comment 43

[Junior [inactive]]

Comment on attachment 8754770 [details] [diff] [review]
Part 4: test, v4

Review of attachment 8754770 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/tests/mochitest/test_presentation_dc_receiver_oop.html
@@ +69,5 @@
> +    }, false);
> +
> +    var promise = new Promise(function(aResolve, aReject) {
> +      document.body.appendChild(receiverIframe);
> +      receiverIframe.addEventListener("mozbrowserloadend", function onLoadEnd() {

(In reply to Junior [:junior] from comment #32)
> Note that there's a crash bustage for Linux x64 opt indicated by comment 24.

I've reproduced locally and find |mozbrowserloadend| is not called if the bustage occurs.
Will try to make them well-ordered

Comment 44

[Junior [inactive]]

> ::: dom/presentation/interfaces/nsIPresentationSessionTransportBuilder.idl
> @@ +24,5 @@
> > +{
> > +  void sendOffer(in nsIPresentationChannelDescription offer);
> > +  void sendAnswer(in nsIPresentationChannelDescription answer);
> > +  void sendIceCandidate(in DOMString candidate);
> > +  void close(in nsresult reason);
> 
> Can we simply add those functions into
> nsIPresentationSessionTransportBuilerListener?

We can do this.
However, nsIPresentationSessionTransportBuilerListener is paired with nsIPresentationSessionTransportBuiler.
This hints us that onOffer/onAnswer... should be in |nsIPresentationSessionTransportBuiler|.
But the derived class PesentationTCPSessionTransportBuilder is not ready to implement this.

That's the reason I put those API in |nsIPresentationDataChannelSessionTransportBuilderListener|

But we can do this although we will have a weird code status for a while.

Comment 45

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Junior [:junior] from comment #42)
> > one suggestion for the patch partitioning:
> > part 1: nsIPresentationSessionTransportBuilder.idl changes - necessary
> > refactory in in-proc data channel handling
> > part 2: PPresentationBuilder.ipdl changes - OOP handling for builder
> > part 3: new test cases for oop data channel scenario
> > 
> part 2 will be a large patch this way.
> Do you think merging would be a good idea since it's easier to trace?
> Or we can still split it to three patches like:
> Part 2-1: implement PresentationBuilder
> Part 2-2: manage session transport in content process
> Part 2-3: hook session transport builder
I found it's hard to figure out the IPC object lifecycle if you split the IPC code in current partition and usually that's the most important part to review.
> 
> > 
> > @@ +21,5 @@
> > > +                            mozIDOMWindow* aWindow, /* unused */
> > > +                            nsIPresentationDataChannelSessionTransportBuilderListener* aListener)
> > > +{
> > > +  RefPtr<PresentationBuilderParent> actor =
> > > +    new PresentationBuilderParent(aRole, aListener);
> > 
> > PresentationOOPDataChannelBuilder and PresentationBuilderParent can be
> > merged into one class.
> > 
> > @@ +140,5 @@
> > > +private:
> > > +  virtual ~PresentationBuilderParent();
> > > +  bool mActorDestroyed = false;
> > > +  nsCOMPtr<nsIPresentationService> mService;
> > > +  RefPtr<PresentationParent> mParent;
> > 
> > PPresentationBuilder is managed by PPresentation so you can call Manager()
> > to get PPresentation object in builder.
> 
> Just a heads-up.
> If we merge PresentationOOPDataChannelBuilder to PresentationBuilderParent,
> the underlying IPC channel will establish by
> |SendPPresentationBuilderConstructor| after |BuildDataChannelTransport|.
> After that, |Manager()| is valid to get |PPresetationParent| object.
> 
> Therefore, I still need to have |RefPtr<PresentationParent> mParent| to send
> the constructor.
> 
ok, keep things that is necessary after the class rearrangment.
> 
> > @@ +440,5 @@
> > > + * In this case, |mBuilder| would be an object of |PresentationParent| and set
> > > + * previously. Therefore, |BuildDataChannelTransport| triggers an IPC call to
> > > + * make content process establish a RTCDataChannel transport.
> > > + */
> > > +bool PresentationSessionInfo::IsOOPDataChannelTransport()
> > 
> > return value doesn't matched with the function name after OnSessionTranport
> > / OnError. Since you only use this function in on place, you can just do an
> > if-check in NotifyOpened.
> > 
> Actually the function is used in two places and I need a good place to put
> these comment.
> How about |return !!mBuilder|?
The problem is that mBuilder will become nullptr after build complete and this function will return true even during the in-proc transport building procedure, this will make |IsOOPDataChannelTransport| not work like its function name. People might try to use it in the future but will not notice the complex precondition of this function.

Comment 46

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Junior [:junior] from comment #44)
> > ::: dom/presentation/interfaces/nsIPresentationSessionTransportBuilder.idl
> > @@ +24,5 @@
> > > +{
> > > +  void sendOffer(in nsIPresentationChannelDescription offer);
> > > +  void sendAnswer(in nsIPresentationChannelDescription answer);
> > > +  void sendIceCandidate(in DOMString candidate);
> > > +  void close(in nsresult reason);
> > 
> > Can we simply add those functions into
> > nsIPresentationSessionTransportBuilerListener?
> 
> We can do this.
> However, nsIPresentationSessionTransportBuilerListener is paired with
> nsIPresentationSessionTransportBuiler.
> This hints us that onOffer/onAnswer... should be in
> |nsIPresentationSessionTransportBuiler|.
> But the derived class PesentationTCPSessionTransportBuilder is not ready to
> implement this.
> 
> That's the reason I put those API in
> |nsIPresentationDataChannelSessionTransportBuilderListener|
> 
> But we can do this although we will have a weird code status for a while.

Just file a bug for merging TCP builder code with latest builder interface.

Comment 47

[Junior [inactive]]

Attachment: Part 1: IPresentationSessionTransportBuilder.idl changes - v1

Comment 48

[Junior [inactive]]

Attachment: Part 2: PPresentationBuilder.ipdl changes

Will file a follow-up bug for the KungFuDeathGrip

Comment 49

[Junior [inactive]]

Attachment: Part 3: new test cases for oop data channel scenario, v5

Carry feedback granted.
We still need to solve the intermittent mentioned in Comment 43.
Upload this patch for reference.

Comment 50

[Junior [inactive]]

Attachment: Part 2: PPresentationBuilder.ipdl changes, v2

forget to inline the IsOOPDataChannelBuilder

Comment 51

[Junior [inactive]]

Attachment: Part 2: PPresentationBuilder.ipdl changes, v3

Upload the right patch

Comment 52

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e53db2275d17

Comment 53

[Junior [inactive]]

Attachment: Part 3: new test cases for oop data channel scenario, v6

Solve the intermittent by loadFrameScript on mozbrowserloadstart instead of mozbrowserloadend. That will promise the mocked behaviour is before the code in inframe executes.

Comment 54

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8755798 [details] [diff] [review]
Part 1: IPresentationSessionTransportBuilder.idl changes - v1

Review of attachment 8755798 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/PresentationDataChannelSessionTransport.js
@@ +47,5 @@
>  
>  PresentationTransportBuilder.prototype = {
>    classID: PRESENTATIONTRANSPORTBUILDER_CID,
>    contractID: PRESENTATIONTRANSPORTBUILDER_CONTRACTID,
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIPresentationDataChannelSessionTransportBuilder,

can |notifyOpened| be removed since we remove the nsIPresentationControlChannelListener?

::: dom/presentation/PresentationSessionInfo.cpp
@@ +468,5 @@
> +  return mControlChannel->Close(reason);
> +}
> +
> +
> +

nit: remove extra new lines

@@ +923,5 @@
> +NS_IMETHODIMP
> +PresentationPresentingInfo::Close(nsresult reason)
> +{
> +  return PresentationSessionInfo::Close(reason);
> +}

looks like PresentationPresentingInfo::OnSessionTransport is the only function we need to override. Can we remove the NS_DECL_NSIPRESENTATIONSESSIONTRANSPORTBUILDERLISTENER and just override the necessary function?

::: dom/presentation/tests/mochitest/test_presentation_datachannel_sessiontransport.html
@@ +156,5 @@
>      serverControlChannel = new TestControlChannel();
>      clientControlChannel.remote = serverControlChannel;
>      serverControlChannel.remote = clientControlChannel;
>  
> +

nit: remove extra new line.

@@ +169,5 @@
> +    clientControlChannel.listener = clientBuilder;
> +    serverListener.sendAnswer = aAnswer=>serverControlChannel.sendAnswer(aAnswer);
> +    serverListener.sendIceCandidate = aCandidate=>serverControlChannel.sendIceCandidate(aCandidate);
> +    serverListener.close = aReason=>serverControlChannel.close(aReason);
> +    serverControlChannel.listener = serverBuilder;

clientControlChannel and serverControlChannel should be removed since the builder interface no longer depend on it.

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

I think it will be easier to review this once there are patches which have the feedback comments fixed.
(and one patch still needs feedback)

Comment 56

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8756168 [details] [diff] [review]
Part 2: PPresentationBuilder.ipdl changes, v3

Review of attachment 8756168 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm for the interface and how function being implemented. I'd like to see the IPDL actor lifecycle issue fixed in next version.

::: dom/presentation/DCPresentationChannelDescription.cpp
@@ +25,5 @@
> +{
> +  if (NS_WARN_IF(!aRetVal)) {
> +    return NS_ERROR_INVALID_POINTER;
> +  }
> +  return NS_OK;

Can we just return NS_ERROR_FAILURE since we don't expect GetTcpAddress / GetTcpPort will be called on this object?

::: dom/presentation/DCPresentationChannelDescription.h
@@ +22,5 @@
> +  {
> +  }
> +
> +private:
> +  ~DCPresentationChannelDescription() {}

virtual

::: dom/presentation/PresentationSessionInfo.cpp
@@ +974,5 @@
>  }
>  
> +// Delegate the pending offer and ICE candidates to builder.
> +NS_IMETHODIMP
> +PresentationPresentingInfo::FlushRequesters(nsIPresentationDataChannelSessionTransportBuilder* builder)

s/FlushRequesters/FlushPendingEvents

@@ +1161,5 @@
>  NS_IMETHODIMP
>  PresentationPresentingInfo::OnIceCandidate(const nsAString& aCandidate)
>  {
>    if (NS_WARN_IF(!mBuilder)) {
> +    mPendingCandidates.AppendElement(nsString(aCandidate));

|!mBuilder| might have two meanings:
1) before builder is created
2) after builder is destroyed

We definitely want to store candidates only for the #1 case.

::: dom/presentation/ipc/PPresentationBuilder.ipdl
@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +sync protocol PPresentationBuilder

async should be enough.

@@ +13,5 @@
> +{
> +  manager PPresentation;
> +
> +parent:
> +  // nsIPresentationControlChannel

this is not for nsIPresentationControlChannel.

@@ +29,5 @@
> +  async OnOffer(nsString aSDP);
> +  async OnAnswer(nsString aSDP);
> +  async OnIceCandidate(nsString aCandidate);
> +
> +  async __delete__(nsresult result);

I didn't see any code calling Send__delete__ on a PresentationBuilderParent, so I'm kinda worry about the reference count handling.
Can you describe the destruction sequence for builder, child, parent object in detail?

::: dom/presentation/ipc/PresentationChild.cpp
@@ +210,5 @@
> +  }
> +
> +  return mBuilder->BuildDataChannelTransport(mRole,
> +                                             window,
> +                                             this /* nsIPresentationSessionTransportBuilderListener */);

this comment can be removed

@@ +269,5 @@
> +
> +  // the building is processing but the IPC of control channel is down
> +  if (NS_WARN_IF(mBuilder)) {
> +    nsresult result = (aResult == NS_OK) ? NS_ERROR_UNEXPECTED : aResult;
> +    NS_WARN_IF(!SendOnSessionTransportError(result));

You cannot send any IPC message on the same actor during the "__delete__" phase.

@@ +282,5 @@
> +{
> +  if (NS_WARN_IF(mActorDestroyed || !SendOnSessionTransport())){
> +    return NS_ERROR_FAILURE;
> +  }
> +  mBuilder = nullptr;

We should move this line after calling NotifySessionTransport since aTransport is a object that created by mBuilder, which is destroyed after this line.

::: dom/presentation/ipc/PresentationChild.h
@@ +42,5 @@
> +
> +  virtual bool
> +  DeallocPPresentationBuilderChild(PPresentationBuilderChild* aActor) override;
> +
> +

nit: remove the extra new line.

@@ +86,4 @@
>    nsCOMPtr<nsIPresentationServiceCallback> mCallback;
>  };
>  
> +class PresentationBuilderChild final: public PPresentationBuilderChild

In separate file please, so does PresentationBuilderParent.

@@ +111,5 @@
> +
> +private:
> +  virtual ~PresentationBuilderChild();
> +
> +  RefPtr<PresentationIPCService> mService;

BuilderChild shouldn't hold a reference to PresentationIPCService. Call |do_GetService| while needed.

::: dom/presentation/ipc/PresentationContentSessionInfo.cpp
@@ +8,5 @@
> +#include "PresentationIPCService.h"
> +
> +/*
> + * Implementation of PresentationContentSessionInfo
> + */

This comment can be removed because it doesn't contain additional information.

::: dom/presentation/ipc/PresentationContentSessionInfo.h
@@ +16,5 @@
> +namespace dom {
> +
> +class PresentationIPCService;
> +
> +class PresentationContentSessionInfo final: public nsIPresentationSessionTransportCallback

nit: space between "final" and ":"

::: dom/presentation/ipc/PresentationIPCService.cpp
@@ +271,5 @@
> +nsresult
> +PresentationIPCService::NotifyTransportClosed(const nsAString& aSessionId,
> +                                              uint8_t aRole,
> +                                              nsresult aReason) {
> +  NS_WARN_IF(!sPresentationChild->SendNotifyTransportClosed(nsString(aSessionId), aRole, aReason));

need to check if the sessionId is actually mapped to a transport on content side?

@@ +349,5 @@
>      mRespondingSessionIds.Remove(windowId);
>    }
>  
> +  RefPtr<PresentationContentSessionInfo> info;
> +  if (mSessionInfos.Get(aSessionId, getter_AddRefs(info))) {

use |Contains| if you don't need the item.

::: dom/presentation/ipc/PresentationParent.cpp
@@ +434,5 @@
> +PresentationBuilderParent::
> +  BuildDataChannelTransport(
> +                  uint8_t aRole,
> +                  mozIDOMWindow* aWindow, /* unused */
> +                  nsIPresentationSessionTransportBuilderListener* aListener)

Still prefer this style: https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsChild.cpp#1665

@@ +446,5 @@
> +                                                               aRole))) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  mParent = nullptr;
> +  mozilla::Unused << actor.forget().take(); // IPDL owns this reference now

adding extra ref count on parent actor means content process will be the controller of the lifecycle of this IPC channel. Is this the semantic we want?

::: dom/presentation/ipc/PresentationParent.h
@@ +47,5 @@
>    virtual bool
>    DeallocPPresentationRequestParent(PPresentationRequestParent* aActor) override;
>  
> +  virtual PPresentationBuilderParent*
> +  AllocPPresentationBuilderParent(const nsString& aSessionId, const uint8_t& aRole) override;

nit: line break after "aSessionId,"

::: dom/presentation/tests/mochitest/test_presentation_sender_startWithDevice.html
@@ +161,5 @@
>    {type: 'presentation-device-manage', allow: true, context: document},
>    {type: 'presentation', allow: true, context: document},
>  ], function() {
>    SpecialPowers.pushPrefEnv({ 'set': [["dom.presentation.enabled", true],
> +                                      ["dom.presentation.session_transport.data_channel.enable", false],

Any reason we can't run this test case with data channel enabled?

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8756253 [details] [diff] [review]
Part 3: new test cases for oop data channel scenario, v6

I'm having hard time to understand the setup here.
I don't understand which port contentScript.sendAsyncMessage uses. Something which has postMessage, but where is that created?

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

oh, I see, loadPrivilegedScript is doing something that I didn't expect.

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8756253 [details] [diff] [review]
Part 3: new test cases for oop data channel scenario, v6

>+    var handlers = {};
>+    addMessageListener = function(message, handler) {
>+      if (handlers.hasOwnProperty(message)) {
>+        handlers[message].push(handler);
>+      } else {
>+        handlers[message] = [handler];
>+      }
>+    };
So we support adding many listeners for same message

>+    removeMessageListener = function(message) {
>+      delete handlers[message];
>+    };
but here we remove all the listeners.
Why this behavior?

Either support adding just one listener, or make removeMessageListener to work the way it is expected.
This is mostly to make the code easier to read.

Looks like the callers of removeMessageListener pass also the listener as second param

Comment 60

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8756168 [details] [diff] [review]
Part 2: PPresentationBuilder.ipdl changes, v3

Review of attachment 8756168 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpref/init/all.js
@@ +5126,5 @@
>  pref("dom.presentation.tcp_server.debug", false);
>  pref("dom.presentation.discovery.enabled", false);
>  pref("dom.presentation.discovery.timeout_ms", 10000);
>  pref("dom.presentation.discoverable", false);
> +pref("dom.presentation.session_transport.data_channel.enable", true);

We cannot default turn on data channel before we have solution for solving backward compatibility. Otherwise video fling from Fennec to TV 2.5 will be broken.

Comment 61

[Junior [inactive]]

Attachment: Part 1: IPresentationSessionTransportBuilder.idl changes - v2

Comment 62

[Junior [inactive]]

Attachment: Part 2: PPresentationBuilder.ipdl changes, v4

Comment 63

[Junior [inactive]]

Attachment: Part 3: new test cases for oop data channel scenario, v7

Comment 64

[Junior [inactive]]

> @@ +29,5 @@
> > +  async OnOffer(nsString aSDP);
> > +  async OnAnswer(nsString aSDP);
> > +  async OnIceCandidate(nsString aCandidate);
> > +
> > +  async __delete__(nsresult result);
> 
> I didn't see any code calling Send__delete__ on a PresentationBuilderParent,
> so I'm kinda worry about the reference count handling.
> Can you describe the destruction sequence for builder, child, parent object
> in detail?
> 
Answer below.
> @@ +446,5 @@
> > +                                                               aRole))) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +  mParent = nullptr;
> > +  mozilla::Unused << actor.forget().take(); // IPDL owns this reference now
> 
> adding extra ref count on parent actor means content process will be the
> controller of the lifecycle of this IPC channel. Is this the semantic we
> want?
> 

After some consideration, I guess this idea would be better:
1) the life cycle of BuilderParent would completely handled by SessionInfo. 
2) the life cycle of BuilderChild is control by the IPDL
The destruction sequence will be:
BuilderParent::OnSessionTransport/OnSessionTransportError 
-> SessionInfo::SetBuilder(nullptr)
-> ~BuilderParent()
-> Send__delete__
--(IPC)--> BuilderChild::Recv__delete__
--(IPDL handle)--> DeallocPresentationBuilderChild

I just observe that the destructor of BuilderChild should have destructed after the Dealloc.
Will continue to find out why.



> :::
> dom/presentation/tests/mochitest/test_presentation_sender_startWithDevice.
> html
> @@ +161,5 @@
> >    {type: 'presentation-device-manage', allow: true, context: document},
> >    {type: 'presentation', allow: true, context: document},
> >  ], function() {
> >    SpecialPowers.pushPrefEnv({ 'set': [["dom.presentation.enabled", true],
> > +                                      ["dom.presentation.session_transport.data_channel.enable", false],
> 
> Any reason we can't run this test case with data channel enabled?

Because the offer-check in the test is TYPE_TCP.
https://dxr.mozilla.org/mozilla-central/rev/b0096c5c727749ad3e79cbdf20d2e96bd179c213/dom/presentation/tests/mochitest/PresentationSessionChromeScript.js#128

Since the pref would be default-off and this check is not essential in this test,
 I guess this pref setting doesn't matter a lot.

Comment 65

[Junior [inactive]]

(In reply to Olli Pettay [:smaug] from comment #59)
> Comment on attachment 8756253 [details] [diff] [review]
> Part 3: new test cases for oop data channel scenario, v6
> 
> >+    var handlers = {};
> >+    addMessageListener = function(message, handler) {
> >+      if (handlers.hasOwnProperty(message)) {
> >+        handlers[message].push(handler);
> >+      } else {
> >+        handlers[message] = [handler];
> >+      }
> >+    };
> So we support adding many listeners for same message
> 
> >+    removeMessageListener = function(message) {
> >+      delete handlers[message];
> >+    };
> but here we remove all the listeners.
> Why this behavior?
> 
> Either support adding just one listener, or make removeMessageListener to
> work the way it is expected.
> This is mostly to make the code easier to read.
> 
> Looks like the callers of removeMessageListener pass also the listener as
> second param

I implement two-params removeMessageListener in next patch.

Comment 66

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Junior [:junior] from comment #64)
> > adding extra ref count on parent actor means content process will be the
> > controller of the lifecycle of this IPC channel. Is this the semantic we
> > want?
> > 
> 
> After some consideration, I guess this idea would be better:
> 1) the life cycle of BuilderParent would completely handled by SessionInfo. 
> 2) the life cycle of BuilderChild is control by the IPDL
> The destruction sequence will be:
> BuilderParent::OnSessionTransport/OnSessionTransportError 
> -> SessionInfo::SetBuilder(nullptr)
> -> ~BuilderParent()
> -> Send__delete__
> --(IPC)--> BuilderChild::Recv__delete__
> --(IPDL handle)--> DeallocPresentationBuilderChild
> 
> I just observe that the destructor of BuilderChild should have destructed
> after the Dealloc.
> Will continue to find out why.
Thanks for thinking this through, however we shouldn't call virtual function during dtor.
You'll probably need to add a shutdown function and invoking Send__delete__ there.
> 
> > :::
> > dom/presentation/tests/mochitest/test_presentation_sender_startWithDevice.
> > html
> > 
> > Any reason we can't run this test case with data channel enabled?
> 
> Because the offer-check in the test is TYPE_TCP.
> https://dxr.mozilla.org/mozilla-central/rev/
> b0096c5c727749ad3e79cbdf20d2e96bd179c213/dom/presentation/tests/mochitest/
> PresentationSessionChromeScript.js#128
> 
> Since the pref would be default-off and this check is not essential in this
> test,
>  I guess this pref setting doesn't matter a lot.

Any chances you can modify this test cases for running with data channel enabled? `startWithDevice` will create a different session info initialization sequence. It is worth to test since you add more steps in session info initialization.

Comment 67

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #66)
> (In reply to Junior [:junior] from comment #64)
> > > adding extra ref count on parent actor means content process will be the
> > > controller of the lifecycle of this IPC channel. Is this the semantic we
> > > want?
> > > 
> > 
> > After some consideration, I guess this idea would be better:
> > 1) the life cycle of BuilderParent would completely handled by SessionInfo. 
> > 2) the life cycle of BuilderChild is control by the IPDL
> > The destruction sequence will be:
> > BuilderParent::OnSessionTransport/OnSessionTransportError 
> > -> SessionInfo::SetBuilder(nullptr)
> > -> ~BuilderParent()
> > -> Send__delete__
> > --(IPC)--> BuilderChild::Recv__delete__
> > --(IPDL handle)--> DeallocPresentationBuilderChild
> > 
> > I just observe that the destructor of BuilderChild should have destructed
> > after the Dealloc.
> > Will continue to find out why.
> Thanks for thinking this through, however we shouldn't call virtual function
> during dtor.
> You'll probably need to add a shutdown function and invoking Send__delete__
> there.
Sorry, Send__delete__ is not a virtual function so you can do just like you proposed.

Comment 68

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8757258 [details] [diff] [review]
Part 2: PPresentationBuilder.ipdl changes, v4

Review of attachment 8757258 [details] [diff] [review]:
-----------------------------------------------------------------

You can start the regular review process after addressing the IPDL shutdown issue and builder reference issue.

::: dom/presentation/DCPresentationChannelDescription.h
@@ +10,5 @@
> +#include "nsIPresentationControlChannel.h"
> +#include "nsString.h"
> +
> +// PresentationChannelDescription for Data Channel
> +class DCPresentationChannelDescription final : public nsIPresentationChannelDescription

put this class in mozilla::dom namespace as well.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +428,5 @@
>  // nsIPresentationSessionTransportBuilderListener
>  NS_IMETHODIMP
>  PresentationSessionInfo::OnSessionTransport(nsIPresentationSessionTransport* transport)
>  {
> +  SetBuilder(nullptr);

This is the root cause why you need kungFuDeathGrip while triggering this callback function.

SessionInfo and builder handle references to each other. Builder invoke |OnSessionTransport| synchronously when transport is created. However, builder might get destructed because the only reference is released by sessionInfo. |this| pointer of builder on the call stack might not point to a valid object after this point. This looks like a pretty bad pattern and people will easily trigger crash in the future.

I think this can be solved by enforcing |OnSessionTransport| to be invoked asynchronously.
The overall procedure will look like this:

1. SessionInfo and builder still hold references to each other.
2. Builder create a runnable to call |OnSessionTransport| asynchronously while transport is created.
3. In |OnSessionTransport|, setup the callback to transport and then, release the reference to builder.

In this case, there will be no dangling |this| pointer on the stack.

::: dom/presentation/ipc/PresentationBuilderChild.cpp
@@ +8,5 @@
> +#include "PresentationBuilderChild.h"
> +#include "PresentationIPCService.h"
> +
> +using namespace mozilla;
> +using namespace mozilla::dom;

declare namespace explicitly without 'using namespace'

@@ +14,5 @@
> +NS_IMPL_ISUPPORTS(PresentationBuilderChild,
> +                  nsIPresentationSessionTransportBuilderListener)
> +
> +PresentationBuilderChild::
> +  PresentationBuilderChild(const nsString& aSessionId,

no need to split into two lines because it is less than 80 chars.

@@ +19,5 @@
> +                           uint8_t aRole)
> +  : mSessionId(aSessionId)
> +  , mRole(aRole)
> +{
> +  MOZ_COUNT_CTOR(PresentationBuilderChild);

We can remote this because IPDL codegen already did this in the parent ctor/dtor.

@@ +22,5 @@
> +{
> +  MOZ_COUNT_CTOR(PresentationBuilderChild);
> +}
> +
> +PresentationBuilderChild::~PresentationBuilderChild()

default destructor is enough

@@ +57,5 @@
> +
> +void
> +PresentationBuilderChild::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  mActorDestroyed = true;

You'll need to release mBuilder in here, otherwise the circular reference between child and builder will keep both objects alive.

@@ +102,5 @@
> +
> +bool
> +PresentationBuilderChild::Recv__delete__()
> +{
> +  return true;

You don't need to override Recv__delete__ since you are doing exactly the same thing.

::: dom/presentation/ipc/PresentationBuilderParent.cpp
@@ +7,5 @@
> +#include "DCPresentationChannelDescription.h"
> +#include "PresentationBuilderParent.h"
> +#include "PresentationSessionInfo.h"
> +
> +using namespace mozilla::dom;

don't do |using namespace|.

@@ +23,5 @@
> +PresentationBuilderParent::~PresentationBuilderParent()
> +{
> +  MOZ_COUNT_DTOR(PresentationBuilderParent);
> +
> +  if (mActorWasActive) {

mActorDestroyed is more common.

::: dom/presentation/ipc/PresentationContentSessionInfo.cpp
@@ +10,5 @@
> +NS_IMPL_ISUPPORTS(PresentationContentSessionInfo,
> +                  nsIPresentationSessionTransportCallback);
> +
> +nsresult
> +PresentationContentSessionInfo::Init() {

weird that this file can get compiled without namespace declaration, probably a side effect of unified build.

::: dom/presentation/ipc/PresentationParent.cpp
@@ +105,5 @@
>    return actor.forget().take();
>  }
>  
>  bool
> +PresentationParent::DeallocPPresentationRequestParent(PPresentationRequestParent* aActor)

This modification is not necessary.

::: dom/presentation/moz.build
@@ +17,2 @@
>      'ipc/PresentationChild.h',
> +    'ipc/PresentationContentSessionInfo.h',

Not need to expose these new header files since they are for local use only.

Comment 69

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8757257 [details] [diff] [review]
Part 1: IPresentationSessionTransportBuilder.idl changes - v2


> PresentationControllingInfo::OnIceCandidate(const nsAString& aCandidate)
> {
>-  MOZ_ASSERT(false, "Should not receive ICE candidates.");
>-  return NS_ERROR_FAILURE;
>+  if (mTransportType != nsIPresentationChannelDescription::TYPE_DATACHANNEL) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  if (NS_WARN_IF(!mBuilder)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder>
>+    builder = do_QueryInterface(mBuilder);
>+  if (NS_WARN_IF(!builder)) {
>+    return NS_ERROR_FAILURE;
>+  }
no need to null check mBuilder. do_QueryInterface is null safe

> PresentationControllingInfo::OnAnswer(nsIPresentationChannelDescription* aDescription)
> {
>+  if (mTransportType == nsIPresentationChannelDescription::TYPE_DATACHANNEL) {
>+
>+    if (NS_WARN_IF(!mBuilder)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+    nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder>
>+      builder = do_QueryInterface(mBuilder);
>+    if (NS_WARN_IF(!builder)) {
>+      return NS_ERROR_FAILURE;
>+    }
ditto

> PresentationControllingInfo::NotifyClosed(nsresult aReason)
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
>+  if (mTransportType == nsIPresentationChannelDescription::TYPE_DATACHANNEL) {
>+    if (NS_WARN_IF(!mBuilder)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+    nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder>
>+      builder = do_QueryInterface(mBuilder);
>+    if (NS_WARN_IF(!builder)) {
>+      return NS_ERROR_FAILURE;
>+    }
and here

> PresentationPresentingInfo::OnIceCandidate(const nsAString& aCandidate)
> {
>-  MOZ_ASSERT(false, "Should not receive ICE candidates.");
>-  return NS_ERROR_FAILURE;
>+  if (NS_WARN_IF(!mBuilder)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder>
>+    builder = do_QueryInterface(mBuilder);
>+  if (NS_WARN_IF(!builder)) {
>+    return NS_ERROR_FAILURE;
>+  }
and here
... and elsewhere too



(not really about this bug, but do we actually need nsIPresentationControlChannelListener, or could that be merged to some other interface PresentationSessionInfo implements?)

Comment 70

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8757259 [details] [diff] [review]
Part 3: new test cases for oop data channel scenario, v7

>+    var handlers = {};
>+    addMessageListener = function(message, handler) {
>+      if (handlers.hasOwnProperty(message)) {
>+        handlers[message].push(handler);
>+      } else {
>+        handlers[message] = [handler];
>+      }
>+    };
>+    removeMessageListener = function(message, handler) {
>+      if (!handler || !handlers.hasOwnProperty(message)) {
>+        return;
>+      }
>+      var index = handlers.indexOf(handler);
how does this work? handlers is {} containing arrays of handlers, not handlers.

Comment 71

[Junior [inactive]]

> @@ +23,5 @@
> > +PresentationBuilderParent::~PresentationBuilderParent()
> > +{
> > +  MOZ_COUNT_DTOR(PresentationBuilderParent);
> > +
> > +  if (mActorWasActive) {
> 
> mActorDestroyed is more common.
> 

The name mActorWasActive is because PresentationBuilderParent isn't an actor after |BuildDataChannelTransport|. Therefore, we should set mActorDestoryed to true in constructor, but it's a little weird since the actor has not been constructed.

Or it's a common name for actor? I can align with it.

Comment 72

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Junior [:junior] from comment #71)
> > @@ +23,5 @@
> > > +PresentationBuilderParent::~PresentationBuilderParent()
> > > +{
> > > +  MOZ_COUNT_DTOR(PresentationBuilderParent);
> > > +
> > > +  if (mActorWasActive) {
> > 
> > mActorDestroyed is more common.
> > 
> 
> The name mActorWasActive is because PresentationBuilderParent isn't an actor
> after |BuildDataChannelTransport|. Therefore, we should set mActorDestoryed
> to true in constructor, but it's a little weird since the actor has not been
> constructed.
> 
> Or it's a common name for actor? I can align with it.

Per our discussion, we can name it something like mNeedDestroyActor to reflect the usage.

Comment 73

[Junior [inactive]]

Attachment: Part 2: PPresentationBuilder.ipdl changes, v5

This patch doesn't have adjust the test mentioned in Comment 66.

Since we have merged OOPBuilder and BuilderParent, will update the class diagram attached in Comment 31.

Comment 74

[Junior [inactive]]

Attachment: Part 3: new test cases for oop data channel scenario, v8

update the implementation of |removeMessageListener|

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8757812 [details] [diff] [review]
Part 2: PPresentationBuilder.ipdl changes, v5



>+  NS_IMETHODIMP
>+  FlushPendingEvents(nsIPresentationDataChannelSessionTransportBuilder* builder);
>+
>+  bool hasFlushPendingEvents = false;
s/hasFlushPendingEvents/mHasFlushPendingEvents/

>   /*
>+   * Notify the transport is closed
>+   *
>+   * @param sessionId: An ID to identify presentation session.
>+   * @param aReason: the error message. NS_OK indicates it is closed normally.
>+   */
>+  void NotifyTransportClosed(in DOMString aSessionId,
>+                             in uint8_t role,
aRole
And you could document also that above.
And the comment talks about sessionId, yet the param name is aSessionId


>   // Store the mapping between the window ID of the OOP page (in this process)
>   // and the ID of the responding session. It's used for an OOP receiver page
>   // to retrieve the correspondent session ID. Besides, also keep the mapping
>   // between the responding session ID and the window ID to help look up the
>   // window ID.
>   nsClassHashtable<nsUint64HashKey, nsString> mRespondingSessionIds;
>   nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;
>+  nsRefPtrHashtable<nsStringHashKey,
>+                    PresentationContentSessionInfo> mSessionInfos;
So PresentationIPCService owns PresentationContentSessionInfo objects and 
PresentationContentSessionInfo has owning pointer to PresentationIPCService.
Is it guaranteed that PresentationIPCService::UntrackSessionInfo is always called?
Could you please document the ownership model here and explain how the cycle is guaranteed to be broken eventually.

+class PresentationContentSessionInfo final : public nsIPresentationSessionTransportCallback
+{
Hmm, so we have PresentationSessionInfo, PresentationControllingInfo and now also PresentationContentSessionInfo.
It is a bit unclear to me from the name of the class what the purpose of PresentationContentSessionInfo is.
If I just think about the name, my guess would be it is actually the SessionInfo on receiver side.
(and PresentationControllingInfo would be the session info on controller side), but then I would be wondering why the name doesn't
talk about receiver.
So I think this needs some clarification, or at least some comments.
Mainly because of this I'd like to see a new patch.


> PresentationRequestParent::DoRequest(const StartSessionRequest& aRequest)
> {
>   MOZ_ASSERT(mService);
>+  RefPtr<PresentationRequestParent> kungFuDeathGrip(this);
Why do you need this?
Remove or add a comment why it is needed.

Comment 76

[Junior [inactive]]

(In reply to Olli Pettay [:smaug] from comment #75)
> Comment on attachment 8757812 [details] [diff] [review]
> Part 2: PPresentationBuilder.ipdl changes, v5
> +class PresentationContentSessionInfo final : public
> nsIPresentationSessionTransportCallback
> +{
> Hmm, so we have PresentationSessionInfo, PresentationControllingInfo and now
> also PresentationContentSessionInfo.
> It is a bit unclear to me from the name of the class what the purpose of
> PresentationContentSessionInfo is.
> If I just think about the name, my guess would be it is actually the
> SessionInfo on receiver side.
> (and PresentationControllingInfo would be the session info on controller
> side), but then I would be wondering why the name doesn't
> talk about receiver.
> So I think this needs some clarification, or at least some comments.
> Mainly because of this I'd like to see a new patch.
> 

A quick heads up: PresentationContentSessionInfo is to manage PresentationDataChannelSessionTransport, who lives in e10s mode and in content process. Plays the role like PresentationSessionInfo in the content process. 

The opposite of |PresentationControllingInfo| is |PresentationPresentingInfo|.
Will update some comments and update the wiki.

Comment 77

[Junior [inactive]]

> 
> >   /*
> >+   * Notify the transport is closed
> >+   *
> >+   * @param sessionId: An ID to identify presentation session.
> >+   * @param aReason: the error message. NS_OK indicates it is closed normally.
> >+   */
> >+  void NotifyTransportClosed(in DOMString aSessionId,
> >+                             in uint8_t role,
> aRole
> And you could document also that above.
> And the comment talks about sessionId, yet the param name is aSessionId
I'd like to change to sessionId, role, and reason to align with this file convention.
Will document |role|
> 
> 
> >   // Store the mapping between the window ID of the OOP page (in this process)
> >   // and the ID of the responding session. It's used for an OOP receiver page
> >   // to retrieve the correspondent session ID. Besides, also keep the mapping
> >   // between the responding session ID and the window ID to help look up the
> >   // window ID.
> >   nsClassHashtable<nsUint64HashKey, nsString> mRespondingSessionIds;
> >   nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;
> >+  nsRefPtrHashtable<nsStringHashKey,
> >+                    PresentationContentSessionInfo> mSessionInfos;
> So PresentationIPCService owns PresentationContentSessionInfo objects and 
> PresentationContentSessionInfo has owning pointer to PresentationIPCService.
> Is it guaranteed that PresentationIPCService::UntrackSessionInfo is always
> called?
> Could you please document the ownership model here and explain how the cycle
> is guaranteed to be broken eventually.
I'll let PresentationContentSessionInfo not own the pointer to PresentationIPCService.
That's will be clearly no cycle.

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Junior [:junior] from comment #76)

> A quick heads up: PresentationContentSessionInfo is to manage
> PresentationDataChannelSessionTransport, who lives in e10s mode and in
> content process. Plays the role like PresentationSessionInfo in the content
> process. 

Somehow the name should capture that information. Hinting about child process perhaps.
"content" is unfortunately so overused term in Gecko.

Comment 79

[Junior [inactive]]

Attachment: Part 2: PPresentationBuilder.ipdl changes, v6

TODO: topics addressed in Comment 73

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8758182 [details] [diff] [review]
Part 2: PPresentationBuilder.ipdl changes, v6

just few nits.

>+PresentationControllingInfo::OnIceCandidate(const nsAString& aCandidate)
>+{
>+  if (mTransportType != nsIPresentationChannelDescription::TYPE_DATACHANNEL) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  if (NS_WARN_IF(!mBuilder)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder>
>+    builder = do_QueryInterface(mBuilder);
So I would probably write this

  nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder>
    builder = do_QueryInterface(mBuilder);
  if (NS_WARN_IF(!builder)) {
    return NS_ERROR_FAILURE;
  }


> PresentationControllingInfo::NotifyClosed(nsresult aReason)
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
>   if (mTransportType == nsIPresentationChannelDescription::TYPE_DATACHANNEL) {
>-    if (NS_WARN_IF(!mBuilder)) {
>-      return NS_ERROR_FAILURE;
>-    }
>-    nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder>
>+    if (mBuilder) {
>+      nsCOMPtr<nsIPresentationDataChannelSessionTransportBuilder>
>       builder = do_QueryInterface(mBuilder);
same here


> 
>   /*
>+   * Notify the transport is closed
>+   *
>+   * @param sessionId: An ID to identify presentation session.
>+   * @param role: Identify the function called by controller or receiver.
>+   * @param reason: the error message. NS_OK indicates it is closed normally.
>+   */
>+  void NotifyTransportClosed(in DOMString aSessionId,
>+                             in uint8_t role,
>+                             in nsresult reason);
So you're inconsistent with param naming.
In general I'd prefer a-prefix, since that is what it should be, but if lowercase is used elsewhere, fine.

Comment 81

[Junior [inactive]]

I have rebased but the test is broken due to conflict with Bug 1258600.

The root cause is |RegisterTransportBuilder| needs SessionInfo in StartSession. But Bug 1258600 postpones the creation of SessionInfo to PresentationDeviceRequst::Select.

I'm working on it.

Comment 82

[Junior [inactive]]

Attachment: Part 1: IPresentationSessionTransportBuilder.idl changes - v3

Comment 83

[Junior [inactive]]

Attachment: Part 2: PPresentationBuilder.ipdl changes, v7

Per comment 81,
we also postpones setting builder after the SessionInfo is created.

The sequence will be:
PresentationDeviceRequest::Select ->
PresentationService::CreateSessionInfo ->
PresentationServiceCallback::ReplySuccess ->
PresentationParent::RegisterTransportBuilder

Will provide interdiff later.

Since the logic has changed, ask for feedback and review again.

Comment 84

[Junior [inactive]]

Attachment: interdiff

Comment 85

[Junior [inactive]]

Attachment: Part 3: new test cases for oop data channel scenario, v9

Comment 86

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88a16da4326c

Comment 87

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8759070 [details] [diff] [review]
Part 2: PPresentationBuilder.ipdl changes, v7

Review of attachment 8759070 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/ipc/PresentationParent.cpp
@@ +270,5 @@
>  {
>    MOZ_ASSERT(mService);
>  
> +  NS_WARN_IF(!RegisterTransportBuilder(aSessionId,
> +                                       nsIPresentationService::ROLE_RECEIVER));

RegisterTransportBuilder always return true so no point to add |NS_WARN_IF|.

Comment 88

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8759070 [details] [diff] [review]
Part 2: PPresentationBuilder.ipdl changes, v7

I don't know why you change
+  virtual ~DCPresentationChannelDescription() {}
to
+  virtual ~DCPresentationChannelDescription() = default;
nor do I know the latter being any better than the former one.
But doesn't matter. up to you.

nit, +PresentationContentSessionInfo::Init() {
{ goes to its own line.


Why the previous version had
+  RefPtr<PresentationParent> parent = static_cast<PresentationParent*>(Manager());
+  if (NS_WARN_IF(!parent->RegisterTransportBuilder(
+                            aRequest.sessionId(),
+                            nsIPresentationService::ROLE_CONTROLLER))) {
+    return NS_ERROR_FAILURE;
+  }
but the new one does...
oh we have now mNeedRegisterBuilder. ok.

Comment 89

[Junior [inactive]]

Attachment: Part 2: PPresentationBuilder.ipdl changes, v8

Modify by reviewer's comment, carry r+ and f+

Only a small nit change, I guess it's no need to run treeherder again

Comment 90

[Gary Chen [:xeonchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25c1b11a4c39
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7a469be511
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b085a46a53

Comment 91

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25c1b11a4c39
https://hg.mozilla.org/mozilla-central/rev/9b7a469be511
https://hg.mozilla.org/mozilla-central/rev/c9b085a46a53

Comment 92

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Don't forget to uplift your patches to TV v2.6 branch.

Comment 93

[Junior [inactive]]

Attachment: OOP Data Channel for presentation API [b2g_v2_6]

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1264513
User impact if declined: Patches for Presentation API needs to base on this patch;
Testing completed: manual testing passed
Risk to taking this patch (and alternatives if risky): No since the pref is default off
String or UUID changes made by this patch: No

Comment 94

[Josh Cheng [:josh]]

Comment on attachment 8762691 [details] [review]
OOP Data Channel for presentation API [b2g_v2_6]

Approve for TV 2.6

Comment 95

[Junior [inactive]]

Hello Gary,
Could you uplift this to b2g48? Thanks :)

Comment 96

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/49b98aa3b415380cc7a021d740b242894654245a