Closed Bug 1299040 Opened 3 years ago Closed 3 years ago

[Presentation WebAPI] Handling reconnect correctly for oop receiver page

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: kershaw, Assigned: kershaw)

References

Details

(Whiteboard: [ETA 9/16])

Attachments

(1 file, 5 obsolete files)

In our current way to handle reconnect at receiver side, |NotifyResponderReady| is called to indicate that the receiver page is ready [1]. However, if the receiver page is oop, we have to call |RegisterTransportBuilder| before |NotifyResponderReady|. Otherwise, it would be considered as an in-process case when building a data channel [2].

[1] http://searchfox.org/mozilla-central/rev/0dd403224a5acb0702bdbf7ff405067f5d29c239/dom/presentation/PresentationService.cpp#397
[2] http://searchfox.org/mozilla-central/rev/0dd403224a5acb0702bdbf7ff405067f5d29c239/dom/presentation/PresentationSessionInfo.cpp#1238
Attachment #8786192 - Flags: feedback?(schien)
Comment on attachment 8786192 [details] [diff] [review]
Better handling reconnect for oop receiver pages

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

@kershaw can you provide a detailed sequence diagram for session reconnect? This patch looks like adding another layer of different path, based on the special assumption of |mBuilder|. I found it hard to give useful feedback without a bird's-eye view of the design.
Attachment #8786192 - Flags: feedback?(schien)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #1)
> Comment on attachment 8786192 [details] [diff] [review]
> Better handling reconnect for oop receiver pages
> 
> Review of attachment 8786192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @kershaw can you provide a detailed sequence diagram for session reconnect?
> This patch looks like adding another layer of different path, based on the
> special assumption of |mBuilder|. I found it hard to give useful feedback
> without a bird's-eye view of the design.

I've created some diagrams below.
Please take a look and let me know what do you think. Thanks.

1. Reconnect flow at controller side: http://goo.gl/Pt65a5
2. Reconnect flow at receiver side for in-process case: http://goo.gl/XFVFrX
3. Reconnect flow at receiver side for oop case: http://goo.gl/Aa97dc
Flags: needinfo?(schien)
Whiteboard: [ETA 9/16]
As we discussed, this patch introduces nsIPresentationSessionTransportBuilder which is responsible for creating session transport builder.

Please take a look. Thanks.
Attachment #8786192 - Attachment is obsolete: true
Flags: needinfo?(schien)
Attachment #8790182 - Flags: feedback?(schien)
Summary:
1. When reconnecting to a connected/connecting connection, close the connection first.
2. Remove notifyReplaced in nsIPresentationSessionListener.
3. Allow to close a connection if it's state is connecting.
Attachment #8790521 - Flags: feedback?(schien)
Comment on attachment 8790521 [details] [diff] [review]
Part 2: Close the connected/connecting connection during reconnecting.

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

This patch should be put on bug 1301259.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +927,5 @@
>      if (nsIPresentationSessionListener::STATE_TERMINATED != mState) {
>        SetStateWithReason(nsIPresentationSessionListener::STATE_CLOSED, aReason);
>      }
>  
> +    if (NS_FAILED(aReason)) {

Add comment to describe the scenario we do |Close| with NS_OK.

@@ +1030,5 @@
> +  if (mState == nsIPresentationSessionListener::STATE_CONNECTED ||
> +      mState == nsIPresentationSessionListener::STATE_CONNECTING) {
> +    NS_WARN_IF(NS_FAILED(
> +      Close(NS_OK, nsIPresentationSessionListener::STATE_CLOSED)));
> +  }

This code means we are doing async transport channel close and async reconnect at the same time.
We need to ensure transport channel close is complete before reestablishing transport channel.

Channel close will trigger |Shutdown| and it will clean up several internal state of session info. Need to ensure the necessary state for session resumption is kept after channel closed.
Attachment #8790521 - Flags: feedback?(schien)
Comment on attachment 8790182 [details] [diff] [review]
Implement a constructor for creating session trnasport builder

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

lgtm!

::: dom/presentation/ipc/PresentationParent.cpp
@@ +17,5 @@
>  #include "PresentationSessionInfo.h"
>  
>  using namespace mozilla::dom;
>  
> +class PresentationTransportBuilderConstructorIPC final :

1. Change the |using namespace| to |namespace|.
2. Put |PresentationTransportBuilderConstructorIPC| in anonymous namespace.
Attachment #8790182 - Flags: feedback?(schien) → feedback+
Summary:
Introduce nsIPresentationTransportBuilderConstructor to make the behavior of creating session transport builder more elegant.

Please have a look. Thanks.
Attachment #8790182 - Attachment is obsolete: true
Attachment #8790521 - Attachment is obsolete: true
Attachment #8790776 - Flags: review?(bugs)
Comment on attachment 8790776 [details] [diff] [review]
Implement a constructor for creating session trnasport builder

Could you just be consistent with argument names. They should be aFoo, not foo.
(there are some 'type' arguments)
Attachment #8790776 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8790776 [details] [diff] [review]
> Implement a constructor for creating session trnasport builder
> 
> Could you just be consistent with argument names. They should be aFoo, not
> foo.
> (there are some 'type' arguments)

Thanks for the detailed review. I didn't even notice that.
It will be fixed in next version.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e684bf18e5d9
Implement transport builder constructor. r=smaug
Keywords: checkin-needed
Fix the error on treeherder.
Attachment #8791602 - Attachment is obsolete: true
Attachment #8792013 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5471f6af44ac
Implement transport builder constructor. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5471f6af44ac
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.