1226144 - [Presentation API] Free sessionId before using it

Status: RESOLVED FIXED

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

Description

[C.M.Chang[:chunmin]]

>PresentationIPCService::MonitorResponderLoading(const nsAString& aSessionId,
>                                                nsIDocShell* aDocShell)
>{
>  MOZ_ASSERT(NS_IsMainThread());
> 
>  mCallback = new PresentationResponderLoadingCallback(aSessionId);
>  return mCallback->Init(aDocShell);
>}

>PresentationIPCService::NotifyReceiverReady(const nsAString& aSessionId,
>                                            uint64_t aWindowId)
>{
>  ...
>  ...
>  mCallback = nullptr;
>  NS_WARN_IF(!sPresentationChild->SendNotifyReceiverReady(nsAutoString(aSessionId)));
>  return NS_OK;
>}

The |aSessionId| passed into |SendNotifyReceiverReady|[1] is holded by |PresentationCallbacks|, which is created in PresentationIPCService::MonitorResponderLoading.

|mCallback = nullptr|[4] above will free the |PresentationCallbacks| and lose |aSessionId| before it's passed into |SendNotifyReceiverReady|[1]

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/ipc/PresentationIPCService.cpp#258
[2] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationCallbacks.h#61
[3] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/ipc/PresentationIPCService.cpp#287
[4] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/ipc/PresentationIPCService.cpp#257

Comment 1

[C.M.Chang[:chunmin]]

Attachment: raw gdb log

Comment 2

[C.M.Chang[:chunmin]]

Attachment: Free presentation sessionId after using it

Hi, Sean,
I was wondering if you could review this small modification

Comment 3

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

Comment on attachment 8689949 [details] [diff] [review]
Free presentation sessionId after using it

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

It looks good to me. Thanks for taking this.

::: dom/presentation/ipc/PresentationIPCService.cpp
@@ +257,2 @@
>    NS_WARN_IF(!sPresentationChild->SendNotifyReceiverReady(nsAutoString(aSessionId)));
> +  mCallback = nullptr;

nit: please add a comment briefly describing why we need to do so. Then it's less likely that someone would make similar mistakes.

Comment 4

[C.M.Chang[:chunmin]]

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

Comment 5

[C.M.Chang[:chunmin]]

Attachment: Bug 1226144 - Free sessionId after using it

Adding a brief comment and carry r+

Comment 6

[C.M.Chang[:chunmin]]

Attachment: bug 1226144 - Free sessionId after using it

fix typo

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a4ed7c96ea4

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a4ed7c96ea4

Comment 9

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

Comment on attachment 8692459 [details] [diff] [review]
bug 1226144 - Free sessionId after using it

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 1069230
User impact if declined: send tab to TV might fail
Testing completed: yes
Risk to taking this patch (and alternatives if risky):  N/A
String or UUID changes made by this patch: N/A

Comment 10

[Josh Cheng [:josh]]

Comment on attachment 8692459 [details] [diff] [review]
bug 1226144 - Free sessionId after using it

Approve for TV 2.5

Comment 11

[Gary Chen [:xeonchen]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/fd9437a5f9a3