1298360 - [Presentation WebAPI] Session ID and window ID mapping should be split to for controller and receiver

Status: RESOLVED FIXED

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

Description

[Kershaw Chang [:kershaw]]

Attachment: WIP

In 1 UA case, the mapping of controller's window ID and session ID [1] would be replaced by the receiver's [2], because both controller and receiver share the same table.
So, we have to separate the table into two parts: one for controller and the other for receiver.

[1] http://searchfox.org/mozilla-central/rev/44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/dom/presentation/PresentationService.cpp#662
[2] http://searchfox.org/mozilla-central/rev/44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/dom/presentation/PresentationService.cpp#943

Comment 1

[Kershaw Chang [:kershaw]]

Comment on attachment 8785249 [details] [diff] [review]
WIP

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

::: dom/presentation/interfaces/nsIPresentationService.idl
@@ -170,5 @@
> -   * @param windowId: The inner window ID used to look up the session ID.
> -   */
> -  DOMString getExistentSessionIdAtLaunch(in unsigned long long windowId);
> -
> -  /*

NOTE: Remove this function since this function is no longer needed.

Comment 2

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

Comment on attachment 8785249 [details] [diff] [review]
WIP

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

Thanks for catching this bug!

::: dom/presentation/PresentationService.cpp
@@ +938,5 @@
>    if (NS_WARN_IF(!info)) {
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> +  AddRespondingSessionId(aWindowId, aSessionId, nsIPresentationService::ROLE_RECEIVER);

nit: break into multiple lines.

::: dom/presentation/PresentationServiceBase.h
@@ +27,5 @@
>  protected:
> +  class SessionIdManager final
> +  {
> +  public:
> +    explicit SessionIdManager() { MOZ_COUNT_CTOR(SessionIdManager); }

nit: split into multiple lines.

@@ +29,5 @@
> +  {
> +  public:
> +    explicit SessionIdManager() { MOZ_COUNT_CTOR(SessionIdManager); }
> +
> +    ~SessionIdManager() { MOZ_COUNT_DTOR(SessionIdManager); }

nit: ditto

@@ +73,3 @@
>    nsresult UpdateWindowIdBySessionIdInternal(const nsAString& aSessionId,
> +                                             const uint64_t aWindowId,
> +                                             uint8_t aRole);

Can we align the order of parameters? I'll prefer (aSessionId, aRole, aWindowId)

::: dom/presentation/ipc/PresentationIPCService.cpp
@@ +396,5 @@
>  
>    if (nsIPresentationService::ROLE_RECEIVER == aRole) {
>      // Terminate receiver page.
>      uint64_t windowId;
> +    if (NS_SUCCEEDED(GetWindowIdBySessionIdInternal(aSessionId, aRole, &windowId))) {

nit: break into multiple lines

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: Separate session id and window id mapping into two parts

Comment 4

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

Comment on attachment 8785521 [details] [diff] [review]
Separate session id and window id mapping into  two parts

Not really about this bug, but do we somehow ensure that
    nsClassHashtable<nsUint64HashKey, nsTArray<nsString>> mRespondingSessionIds;
    nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;
stay in sync?
I mean that if WindowID maps to some sessionID, that same sessionID maps to that window?

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8785521 [details] [diff] [review]
> Separate session id and window id mapping into  two parts
> 
> Not really about this bug, but do we somehow ensure that
>     nsClassHashtable<nsUint64HashKey, nsTArray<nsString>>
> mRespondingSessionIds;
>     nsDataHashtable<nsStringHashKey, uint64_t> mRespondingWindowIds;
> stay in sync?
> I mean that if WindowID maps to some sessionID, that same sessionID maps to
> that window?

Yes, I think these two tables are synced.
Right now, all operations to the tables have to be through SessionIdManager, so I think we can make sure the synchronization.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Separate session id and window id mapping into two parts, r=smaug

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cc009739cd
Separate session id and window id mapping into two parts. r=smaug

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8cc009739cd