Closed Bug 1264513 Opened 4 years ago Closed 3 years ago

[Presentation WebAPI] Enable OOP RTCDataChannel session transport

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
blocking-b2g 2.6+
Tracking Status
firefox49 --- fixed
b2g-v2.6 --- fixed

People

(Reporter: junior, Assigned: junior)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [ETA 5/26] btpp-active)

Attachments

(6 files, 32 obsolete files)

99.75 KB, image/jpeg
Details
25.09 KB, patch
junior
: review+
junior
: feedback+
Details | Diff | Splinter Review
6.02 KB, patch
Details | Diff | Splinter Review
30.29 KB, patch
junior
: review+
junior
: feedback+
Details | Diff | Splinter Review
78.71 KB, patch
junior
: review+
junior
: feedback+
Details | Diff | Splinter Review
48 bytes, text/x-github-pull-request
Details | Review
We need to enable the oop part.
Depends on: 1260076
Assignee: nobody → juhsu
Since bug 1257734 seems not going to land, I change the sender test to use loadPrivilegedScript (bug 1264513)
Attachment #8742178 - Attachment is obsolete: true
loadFrameScript enables the receiver_oop with some bustage
Attachment #8742726 - Attachment is obsolete: true
Finish receiver oop test.
TODO: test controlchannel IPC
Attachment #8743711 - Attachment is obsolete: true
Attachment #8743743 - Attachment is patch: true
test IPC
TODO: enrich test coverage and error handling
Attachment #8743743 - Attachment is obsolete: true
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.
Attachment #8744262 - Attachment is obsolete: true
Attachment #8745286 - Flags: review?(bugs)
uh, that is quite some large patch ;)
It might be worth to split tests to a separate patch
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.
Attachment #8745286 - Flags: review?(bugs)
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.
Whiteboard: [ETA 5/26]
blocking-b2g: --- → 2.6?
Have solve the crash issue and leakage.
But hit the assertion
Assertion failure: typeDescrObjects.empty(), at js/src/gc/Zone.cpp:65
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.
Attachment #8745286 - Attachment is obsolete: true
Attachment #8751218 - Flags: review?(bugs)
Attachment #8751218 - Flags: feedback?(schien)
Let IPCService handle the OOP session transport
Attachment #8751221 - Flags: review?(bugs)
Attachment #8751221 - Flags: feedback?(schien)
Session info will start to use PresentationParent as a data channel builder.
Attachment #8751224 - Flags: review?(bugs)
Attachment #8751224 - Flags: feedback?(schien)
Attached patch Part4: test, v1 (obsolete) — Splinter Review
Attachment #8751225 - Flags: review?(bugs)
Attachment #8751225 - Flags: feedback?(schien)
Note that we still hit the assertion mentioned in Comment 12.
Attached patch Part 4: test, v2 (obsolete) — Splinter Review
fix a nit
Attachment #8751225 - Attachment is obsolete: true
Attachment #8751225 - Flags: review?(bugs)
Attachment #8751225 - Flags: feedback?(schien)
Attachment #8751236 - Flags: review?(bugs)
Attachment #8751236 - Flags: feedback?(schien)
Reviews are coming. Sorry about delay. Trying to get to this and other presentation API patches tomorrow.
Attachment #8751218 - Attachment is obsolete: true
Attachment #8751218 - Flags: review?(bugs)
Attachment #8751218 - Flags: feedback?(schien)
Attachment #8752154 - Flags: review?(bugs)
Attachment #8752154 - Flags: feedback?(schien)
Attached patch Part 4: test, v3 (obsolete) — Splinter Review
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
Attachment #8751236 - Attachment is obsolete: true
Attachment #8751236 - Flags: review?(bugs)
Attachment #8751236 - Flags: feedback?(schien)
Attachment #8752155 - Flags: review?(bugs)
Attachment #8752155 - Flags: feedback?(schien)
Remove the wrong modification.

And it's confirmed that the assertion in Comment 12 is solved by removing the leakage.
Attachment #8752154 - Attachment is obsolete: true
Attachment #8752154 - Flags: review?(bugs)
Attachment #8752154 - Flags: feedback?(schien)
Attachment #8752159 - Flags: review?(bugs)
Attachment #8752159 - Flags: feedback?(schien)
Perhaps schien could give feedback before a review.
(In reply to Olli Pettay [:smaug] from comment #26)
> Perhaps schien could give feedback before a review.

I plan to do this today.
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.
Attachment #8752159 - Flags: feedback?(schien)
This is the IPC boundary I have in mind. We should be able to isolate the code by making TransportBuilder interface IPC-friendly.
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)
Attachment #8752159 - Flags: review?(bugs)
Attached image ClassDiagram.jpg
The class diagram during the transport building process
Note that there's a crash bustage for Linux x64 opt indicated by command 24.
blocking-b2g: 2.6? → 2.6+
Attachment #8752159 - Attachment is obsolete: true
Attachment #8754766 - Flags: feedback?(schien)
Attachment #8751221 - Attachment is obsolete: true
Attachment #8751221 - Flags: review?(bugs)
Attachment #8751221 - Flags: feedback?(schien)
Attachment #8754767 - Flags: feedback?(schien)
Attachment #8751224 - Attachment is obsolete: true
Attachment #8751224 - Flags: review?(bugs)
Attachment #8751224 - Flags: feedback?(schien)
Attachment #8754768 - Flags: feedback?(schien)
Attached patch Part 4: test, v4 (obsolete) — Splinter Review
Attachment #8752155 - Attachment is obsolete: true
Attachment #8752155 - Flags: review?(bugs)
Attachment #8752155 - Flags: feedback?(schien)
Attachment #8754770 - Flags: feedback?(schien)
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.
Attachment #8754766 - Flags: feedback?(schien)
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
Attachment #8754767 - Flags: feedback?(schien) → feedback+
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.
Attachment #8754768 - Flags: feedback?(schien)
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.
Attachment #8754770 - Flags: feedback?(schien) → feedback+
> 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|?
Flags: needinfo?(schien)
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
> ::: 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.
(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.
Flags: needinfo?(schien)
(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.
Blocks: 1275150
Attachment #8754766 - Attachment is obsolete: true
Attachment #8755798 - Flags: review?(bugs)
Attachment #8755798 - Flags: feedback?(schien)
Will file a follow-up bug for the KungFuDeathGrip
Attachment #8754767 - Attachment is obsolete: true
Attachment #8754768 - Attachment is obsolete: true
Attachment #8755800 - Flags: review?(bugs)
Attachment #8755800 - Flags: feedback?(schien)
Carry feedback granted.
We still need to solve the intermittent mentioned in Comment 43.
Upload this patch for reference.
Attachment #8754770 - Attachment is obsolete: true
Attachment #8755801 - Flags: feedback+
forget to inline the IsOOPDataChannelBuilder
Attachment #8755800 - Attachment is obsolete: true
Attachment #8755800 - Flags: review?(bugs)
Attachment #8755800 - Flags: feedback?(schien)
Attachment #8755814 - Flags: review?(bugs)
Attachment #8755814 - Flags: feedback?(schien)
Upload the right patch
Attachment #8755814 - Attachment is obsolete: true
Attachment #8755814 - Flags: review?(bugs)
Attachment #8755814 - Flags: feedback?(schien)
Attachment #8756168 - Flags: review?(bugs)
Attachment #8756168 - Flags: feedback?(schien)
Solve the intermittent by loadFrameScript on mozbrowserloadstart instead of mozbrowserloadend. That will promise the mocked behaviour is before the code in inframe executes.
Attachment #8755801 - Attachment is obsolete: true
Attachment #8756253 - Flags: review?(bugs)
Attachment #8756253 - Flags: feedback+
Component: DOM: Core & HTML → DOM
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.
Attachment #8755798 - Flags: feedback?(schien) → feedback+
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 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?
Attachment #8756168 - Flags: feedback?(schien)
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?
oh, I see, loadPrivilegedScript is doing something that I didn't expect.
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 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.
Attachment #8755798 - Attachment is obsolete: true
Attachment #8755798 - Flags: review?(bugs)
Attachment #8757257 - Flags: review?(bugs)
Attachment #8757257 - Flags: feedback+
Attachment #8756168 - Attachment is obsolete: true
Attachment #8756168 - Flags: review?(bugs)
Attachment #8757258 - Flags: feedback?(schien)
Attachment #8756253 - Attachment is obsolete: true
Attachment #8756253 - Flags: review?(bugs)
Attachment #8757259 - Flags: review?(bugs)
Attachment #8757259 - Flags: feedback+
> @@ +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.
(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.
(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.
(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 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.
Attachment #8757258 - Flags: feedback?(schien) → feedback+
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?)
Attachment #8757257 - Flags: review?(bugs) → review+
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.
Attachment #8757259 - Flags: review?(bugs) → review-
> @@ +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.
Flags: needinfo?(schien)
(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.
Flags: needinfo?(schien)
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.
Attachment #8757258 - Attachment is obsolete: true
Attachment #8757812 - Flags: review?(bugs)
Attachment #8757812 - Flags: feedback+
update the implementation of |removeMessageListener|
Attachment #8757259 - Attachment is obsolete: true
Attachment #8757817 - Flags: review?(bugs)
Attachment #8757817 - Flags: feedback+
Whiteboard: [ETA 5/26] → [ETA 5/26] btpp-active
Attachment #8757817 - Flags: review?(bugs) → review+
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.
Attachment #8757812 - Flags: review?(bugs) → review-
(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.
> 
> >   /*
> >+   * 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.
(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.
Blocks: 1276883
TODO: topics addressed in Comment 73
Attachment #8757812 - Attachment is obsolete: true
Attachment #8758182 - Flags: review?(bugs)
Attachment #8758182 - Flags: feedback+
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.
Attachment #8758182 - Flags: review?(bugs) → review+
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.
Attachment #8757257 - Attachment is obsolete: true
Attachment #8759068 - Flags: review+
Attachment #8759068 - Flags: feedback+
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.
Attachment #8758182 - Attachment is obsolete: true
Attachment #8759070 - Flags: review?(bugs)
Attachment #8759070 - Flags: feedback?(schien)
Attached patch interdiffSplinter Review
Attachment #8757817 - Attachment is obsolete: true
Attachment #8759074 - Flags: review+
Attachment #8759074 - Flags: feedback+
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|.
Attachment #8759070 - Flags: feedback?(schien) → feedback+
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.
Attachment #8759070 - Flags: review?(bugs) → review+
Modify by reviewer's comment, carry r+ and f+

Only a small nit change, I guess it's no need to run treeherder again
Attachment #8759070 - Attachment is obsolete: true
Attachment #8759474 - Flags: review+
Attachment #8759474 - Flags: feedback+
Keywords: checkin-needed
Don't forget to uplift your patches to TV v2.6 branch.
Flags: needinfo?(juhsu)
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
Flags: needinfo?(juhsu)
Attachment #8762691 - Flags: approval-mozilla-b2g48?
Comment on attachment 8762691 [details] [review]
OOP Data Channel for presentation API [b2g_v2_6]

Approve for TV 2.6
Attachment #8762691 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Hello Gary,
Could you uplift this to b2g48? Thanks :)
Flags: needinfo?(xeonchen)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.