Closed Bug 1298360 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: kershaw, Assigned: kershaw)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch WIP (obsolete) — Splinter Review
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
Attachment #8785249 - Flags: feedback?(schien)
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 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
Attachment #8785249 - Flags: feedback?(schien) → feedback+
Attachment #8785249 - Attachment is obsolete: true
Attachment #8785521 - Flags: review?(bugs)
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?
Attachment #8785521 - Flags: review?(bugs) → review+
(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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cc009739cd
Separate session id and window id mapping into two parts. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a8cc009739cd
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.