1192727 - [Presentation WebAPI] Improve the way that Presentation receiver gets the ID of the incoming session

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Currently the following is used to bind the opened receiver page and its correspondent session ID when a presentation session request comes in:
1. Presentation service (at B2G process) has a variable |mRespondingSessionId| to track the session ID . [1]
2. The opened web page (content process) makes a synced IPC call |GetExistentSessionIdAtLaunch| [2] to B2G process and determines it as a receiver if |mRespondingSessionId| is available. [3]
3. Then |mRespondingSessionId| gets truncated after the correspondent receiver page calls |RegisterSessionListener|. [4]

However, this mechanism could introduce some issues described as follows:

A. If another content process happens to call |GetExistentSessionIdAtLaunch| between step 1 and 3. Then it might accidentally "steal" the session ID which is supposed for the real receiver page in another process, and thus mistakenly regards itself as a receiver. And the real receiver page might fail to create a correspondent session instance.

B. If the receiver page never accesses |navigator.presentation|, no more incoming presentation session request can be handled since there's no |RegisterSessionListener| call to reset |mRespondingSessionId| (unless it gets stolen by another page).

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationService.cpp?offset=200#314-318
[2] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/ipc/PPresentation.ipdl?offset=100#61
[3] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/Presentation.cpp?offset=0#80-84
[4] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationService.cpp?offset=0#486-488

Comment 1

[Sean Lin [:seanlin] (inactive from Mar 2016)]

[Blocking Requested - why for this release]:

Comment 2

[Sean Lin [:seanlin] (inactive from Mar 2016)]

A possible fix could be as follows:

1. Actually the session ID is passed to the content process in a |NotifyPresentationReceiverLaunched| IPC call. [1] Instead of keeping tracking of |mRespondingSessionId| in B2G process, |PresentationIPCService| in the content process may store the mapping info between the window ID of the opened receiver iframe (outer window), the session ID, and the presenting URI. (It's probably necessary to carry the presenting URL in the |NotifyPresentationReceiverLaunched| call.)

2. When it comes to check the existent session ID at launch [2], we can get the outer window of the current document. And then use its ID as well as the document URI to get the correspondent session ID (if any). So fetching the session ID is protected by checking the outer window ID and the URI. Once the session ID gets fetched, remove its mapping to ensure no more page can take it.

3. |PresentationService| at B2G process can do similar things for in-process receiver frames. The synced IPC call |GetExistentSessionIdAtLaunch| [3] becomes no longer necessary.

4. When the (responder) session info at B2G process should be removed (e.g. session close), make an async IPC call to the content process to clean up the unused session ID (if any).

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#664
[2] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/Presentation.cpp?offset=0#80-84
[3] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/ipc/PPresentation.ipdl?offset=100#61


Hi smaug,

Could you take a look and share your feedback about this proposal?

Comment 3

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

about (1): Should also parent process keep the ID and such, so that a compromised child process can't fake the ID. Parent would always validate. Or perhaps parent process should just have
per-child-process ID?

I think otherwise that looks ok.

Comment 4

[Sean Lin [:seanlin] (inactive from Mar 2016)]

[Tracking Requested - why for this release]:

Comment 5

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch, v1

Comment 6

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

Comment on attachment 8649623 [details] [diff] [review]
Patch, v1

I'd like to see some comments in the code, and
it is rather hard to figure out what mRespondingInfo means in which case.
It is used both in PresentationIPCService and PresentationService.
What thing runs in which process and when?
I think the same variable names shouldn't be used in those two different cases.

This all could use some short description somewhere explaining the communication between different processes.

Comment 7

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch, v2

Comment 8

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

Comment on attachment 8652218 [details] [diff] [review]
Patch, v2


>+    // Notify the content process that a receiver page has launched, so it can
Which content process? sender or receiver? Please clarify

>   nsRefPtr<PresentationResponderLoadingCallback> mLoadingCallback;
>   nsCOMPtr<nsITimer> mTimer;
>   nsCOMPtr<nsIPresentationChannelDescription> mRequesterDescription;
>   nsRefPtr<Promise> mPromise;
>+  nsCOMPtr<nsIContentParent> mContentParent;
Which ContentParent is this about? 

>+  uint64_t mWindowId;
Which WindowID is this about. Something in sender or receiver?


>+PresentationIPCService::GetExistentSessionIdAtLaunch(uint64_t aWindowId,
>+                                                     nsIURI* aUri,
>+                                                     nsAString& aSessionId)
I think we should rename the method, since it isn't anymore just "Get", but it is more like
TakeExistingSessionIdAtLaunch() given that the method calls RemoveRespondingInfo

>   /*
>    * Check if the presentation instance has an existent session ID at launch.
>-   * An empty string is returned at sender side; non-empty at receiver side.
>+   * An empty string is always returned at sender side. Whereas at receiver side
>+   * the associated session ID is returned if the window ID and URI are matched;
>+   * otherwise an empty string is returned.
>+   *
>+   * @param windowId: The window ID used to look up the session ID.
>+   * @param uri: The URI used to look up the session ID.
>    */
>-  DOMString getExistentSessionIdAtLaunch();
>+  DOMString getExistentSessionIdAtLaunch(in uint64_t windowId,
>+                                         in nsIURI uri);
Oh, MonitorResponderLoading is called in ContentChild::RecvNotifyPresentationReceiverLaunched which is receiver side?
RecvNotifyPresentationReceiverLaunched so much sounds like something to be called on the sender side notifying that receiver has been launched.


I wonder about the Window ID handling. I think we need to use inner windows, not outer windows, so that if a new page is loaded, the same id isn't reused.
nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(aDocShell); gives you an outer window, and from that GetCurrentInnerWindow() gives the inner window.

Comment 9

[Sean Lin [:seanlin] (inactive from Mar 2016)]

(In reply to Olli Pettay [:smaug] (high review load) from comment #8)
> Comment on attachment 8652218 [details] [diff] [review]
> Patch, v2
> >+    // Notify the content process that a receiver page has launched, so it can
> Which content process? sender or receiver? Please clarify
> 
The receiver side. |PresentationResponderInfo| objects are only created and running at the receiver side. (Meanwhile the sender side uses |PresentationRequesterInfo|.)

> >   nsRefPtr<PresentationResponderLoadingCallback> mLoadingCallback;
> >   nsCOMPtr<nsITimer> mTimer;
> >   nsCOMPtr<nsIPresentationChannelDescription> mRequesterDescription;
> >   nsRefPtr<Promise> mPromise;
> >+  nsCOMPtr<nsIContentParent> mContentParent;
> Which ContentParent is this about? 
> 
The content parent communicating with the content process which the receiver frame belongs to. We keep it, so there's a way to determine if the responding info is store in a content process (for OOP frames) or the parent process (for in-process frame), when it comes to remove the responding info in |PresentationResponderInfo::UntrackFromService| (if it hasn't been taken and removed by checking the existent session ID).

I'll add some comments for this.

> >+  uint64_t mWindowId;
> Which WindowID is this about. Something in sender or receiver?
> 
The receiver side. |PresentationResponderInfo| objects are only created and running at the receiver side.

> 
> >+PresentationIPCService::GetExistentSessionIdAtLaunch(uint64_t aWindowId,
> >+                                                     nsIURI* aUri,
> >+                                                     nsAString& aSessionId)
> I think we should rename the method, since it isn't anymore just "Get", but
> it is more like
> TakeExistingSessionIdAtLaunch() given that the method calls
> RemoveRespondingInfo
>
Will do.
 
> >   /*
> >    * Check if the presentation instance has an existent session ID at launch.
> >-   * An empty string is returned at sender side; non-empty at receiver side.
> >+   * An empty string is always returned at sender side. Whereas at receiver side
> >+   * the associated session ID is returned if the window ID and URI are matched;
> >+   * otherwise an empty string is returned.
> >+   *
> >+   * @param windowId: The window ID used to look up the session ID.
> >+   * @param uri: The URI used to look up the session ID.
> >    */
> >-  DOMString getExistentSessionIdAtLaunch();
> >+  DOMString getExistentSessionIdAtLaunch(in uint64_t windowId,
> >+                                         in nsIURI uri);
> Oh, MonitorResponderLoading is called in
> ContentChild::RecvNotifyPresentationReceiverLaunched which is receiver side?
> RecvNotifyPresentationReceiverLaunched so much sounds like something to be
> called on the sender side notifying that receiver has been launched.
> 
Yep, it's at the receiver side. |NotifyPresentationReceiverLaunched| is to notify the content process that a receiver (frame) has just been launched.

> 
> I wonder about the Window ID handling. I think we need to use inner windows,
> not outer windows, so that if a new page is loaded, the same id isn't reused.
> nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(aDocShell); gives you an
> outer window, and from that GetCurrentInnerWindow() gives the inner window.

Actually I was thinking of this too. But after a quick test I realized the inner window could be for "about:blank" at this moment and was different from the one for the real receiver page. That's why an outer window is used here along with an URI to help identify the receiver page.

Yet I just figured out a way to use the inner window without having the issue above. We may wait until the loading callback to call |NotifyReceiverReady|, and then use the inner window, which should represent the real receiver page at this time.

Comment 10

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch, v3

Updating based on comment 8 and comment 9.

Comment 11

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

Comment on attachment 8653397 [details] [diff] [review]
Patch, v3

> PresentationResponderLoadingCallback::NotifyReceiverReady()
> {
>+  nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(mProgress);
>+  if (NS_WARN_IF(!window)) {
>+    return NS_ERROR_NOT_AVAILABLE;
>+  }
>+
>+  nsCOMPtr<nsISupportsPRUint64> windowId =
>+    do_CreateInstance(NS_SUPPORTS_PRUINT64_CONTRACTID);
>+  if (NS_WARN_IF(!windowId)) {
>+    return NS_ERROR_FAILURE;
>+  }
>+  windowId->SetData(window->GetCurrentInnerWindow()->WindowID());
Null check the return value of GetCurrentInnerWindow()
>+PresentationIPCService::NotifyReceiverReady(const nsAString& aSessionId,
>+                                            nsISupportsPRUint64* aWindowId)
It is a bit unusual to use nsISupportsPRuint64, but since you want to be able to pass null, fine.

But since the documentation says passing null is fine, please null check aWindowId also in 
PresentationIPCService case.

Comment 12

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch, v4

Updating based on r+ comments.

Besides, since no actual window ends up with 0 as its ID [1], passing 0 to |uint64_t aWindowId| can act similarly as if nullptr is passed to |nsISupportsPRUint64* aWindowId|. So I've also changed |NotifyReceiverReady| to use |uint64_t| for window ID parameter.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp?offset=0#2801

Comment 13

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Try run. FYI.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=789c57c8ab22

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Patch doesn't apply cleanly. Please rebase on top of b2g-inbound tip.

Comment 15

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch, v4 (rebased)

Comment 16

[Sean Lin [:seanlin] (inactive from Mar 2016)]

The latest try run for the rebased patch. Thank you.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b4d45f4d370

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/24b78ae6d1a0

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/24b78ae6d1a0