1234128 - [Presentation WebAPI] navigator.presentation.receiver is null in 1-UA use case.

Status: RESOLVED FIXED

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

Description

[Tommy Kuo (away forever...)]

I found navigator.presentation.receiver is null when using Presentation API in 1-UA use case. The reason is PresentationIPCService::GetExistentSessionIdAtLaunch called before PresentationIPCService::NotifyReceiverReady. So PresentationIPCService::GetExistentSessionIdAtLaunch got an empty session id and does not create PresentationReceiver.

A workaround is using navigator.presentation with 1 sec delay.

Comment 1

[Tommy Kuo (away forever...)]

I noticed that it need longer time to call PresentationIPCService::NotifyReceiverReady. We need to go thru the following steps before calling PresentationIPCService::NotifyReceiverReady:

1. App launched
2. Notify shell_remote.js
3. Notify observer (PresentationRequestUIGlue.js)
4. Call PresentationIPCService::NotifyReceiverReady

And navigator.presentation.receiver is usually used when app launched.

Comment 2

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

In bug 1245031, the same timing issue is confirmed to happen to 2-UA scenarios under some edge cases. And it's intermittently reproducible in mochitest run on the local desktop (Linux x64). As a short-term workaround, we disable the affected test and wait for the fix to this bug to enable it again.

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add observer to Presentation in order to detect prepare of receiver.

Hi Tommy,

Could you please confirm this patch?

I encountered this bug at bug 1217713, comment 14.(Simulate of Presentation API)
I added the observer in order to detect status of receiver for testing. The patch of those testing is this attachment.

I thought that |PresentationPresentingInfo#NotifyResponderready| is called when receiver is ready, even if frame is OOP or in-process. So I think that we can observe those function as whether receiver is ready.

Comment 4

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

Comment on attachment 8731565 [details] [diff] [review]
Add observer to Presentation in order to detect prepare of receiver.

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

Presentation and PresentationSessionInfo object can be in different process, which cannot be communicated over observer.

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Many thanks schine.

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4)
> Comment on attachment 8731565 [details] [diff] [review]
> Add observer to Presentation in order to detect prepare of receiver.
> 
> Review of attachment 8731565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Presentation and PresentationSessionInfo object can be in different process,
> which cannot be communicated over observer.
I think that |PresentationPresentingInfo#NotifyResponderReady| called from content process. 
In OOP case(Gonk/Desktop), notify to content process from parent process via IPC[1]. So We can detect receiver ready status in content process.
In process frame case(Mulet), notify to same process as function call[2].
I guess that we can notify receiver status to content process using current presentation implementation.

I am sorry if my interpretation is mistaken.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#963
[2] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#975

Comment 6

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

PresentationSessionInfo object always lives in chrome process and Presentation object lives in the same process as the receiver web page (content process for e10s and chrome process for Mulet).

For e10s environment the notification is just like what you described, and for non-e10s environment the notification is done at the same function at [1]. The entire flow is like below:

PresentationSessionInfo::ResolvedCallback
[=> IPC to content process for e10s]
=> PresentationResponderLoadingCallback::Init
[=> wait for document loading callback if necessary]
=> PresentationResponderLoadingCallback::NotifyReceiverReady
=> nsIPresentationService::NotifyReceiverReady
  => mResponderSessionIds.Put(windowId, sessionId)
[=> IPC to chrome process for e10s]
=> PresentationSessionInfo::NotifyResponderReady

As for content page, navigator.presentation object is created at the first use and we decide whether this page is for controller and receiver. The check we use is via nsIPresentationService::GetExistingSessionIdAtLaunch and it checks by mResponderSessionIds.Get(windowId).

So you can see there is a timing issue if the first use of navigator.presentation comes earlier than mResponderSessionIds.Put().

That's why I think your patch doesn't matched with the current code and might not solve the bug. The real solution I can imaging is either to improve the controller/receiver check, or do synchronous check while creating PresentationReceiver object.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/presentation/PresentationSessionInfo.cpp#974

Comment 7

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

(FWIW, I think it would be great if someone draw some picture of the object hierarchy in Presentation API. And also some state diagram. Perhaps separate versions for non-OOP and OOP. )

Comment 8

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

(In reply to Olli Pettay [:smaug] from comment #7)
> (FWIW, I think it would be great if someone draw some picture of the object
> hierarchy in Presentation API. And also some state diagram. Perhaps separate
> versions for non-OOP and OOP. )

I put the sequence diagram on https://wiki.mozilla.org/WebAPI/PresentationAPI:CoreService#handle_presentation_request .
I'll take first stab on this bug.

Comment 9

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

The solution I have in mind is to pass a flag while creating the browser element for presented content, i.e. we'll need to introduce additional 'mozPresentation' attributes in browser element. Therefore, this information will be ready at the very beginning of document loading procedure.

Comment 10

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

Attachment: [WIP] mark-as-receiver-at-iframe-launching.patch

WIP for marking presented content with "mozPresentation" attribute.

TODOs
- support non-remote iframe
- check same-origin policy for iframe inside the presented content

Comment 11

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

Attachment: mark-as-receiver-at-iframe-launching.patch

Introduce 'mozpresentation' attribute for browser element for identifying the scope of presented content.

Comment 12

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

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

Comment 13

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

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

Comment 14

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

Attachment: part 1, introduce mozpresentation

add "mozpresentation=<requested URL>" in browser element.

Comment 15

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

Attachment: part 2, use presentation URL to identify prsented content

use the presentation URL stored in nsDocShell to identify the window for presented content.

Comment 16

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

Attachment: part 3, test cases

update test cases to use mozbrowser and mozpresentation.

Comment 17

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

Reviews are coming. Sorry about delay. Trying to get to this and other presentation API patches tomorrow.

Comment 18

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

Comment on attachment 8750169 [details] [diff] [review]
part 1, introduce mozpresentation

Could we not store presentation url in so many places.
in non-OOP case one can access is from the xul:browser and in IPC case from TabChild.
So is there any reason to have setPresentationURL, when GetPresentationURL can get the information from elsewhere.

And if there is...
readonly attribute DOMString presentationURL;
void setPresentationURL(in DOMString presentationURL);
is effectively the same as 
attribute DOMString presentationURL;


If we want to keep nsDocShell::SetPresentationURL, it should want if it is used on non-toplevel root same-type-docshell.


...but let me read other patches.

Comment 19

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

Comment on attachment 8750169 [details] [diff] [review]
part 1, introduce mozpresentation

Yeah, so after looking at the other patch, I think we don't need any changes to docshell. Just some helper method which takes a docshell as param and then tries to access TabChild from it and the presentation url from it, or in non-OOP case there seems to be the helper GetTopFrameElement.

I just prefer to not add presentation API specific stuff to docshell if possible, since docshell is complicated enough

Comment 20

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

Comment on attachment 8750170 [details] [diff] [review]
part 2, use presentation URL to identify prsented content

So docshell->GetPresentationURL calls would need to use some helper method, but otherwise look ok.

Comment 21

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

Comment on attachment 8750171 [details] [diff] [review]
part 3, test cases

I'm pretty sure you don't want alert() anywhere.
Wouldn't console.log() or dump() work better than alert.

mostly rs+

Comment 22

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

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8750171 [details] [diff] [review]
> part 3, test cases
> 
> I'm pretty sure you don't want alert() anywhere.
> Wouldn't console.log() or dump() work better than alert.
> 
> mostly rs+

|alert| is used as a content-to-chrome IPC mechanism (via mozBrowser API) in these test cases.

Comment 23

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

I see. ok, thanks.

Comment 24

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

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

Comment 25

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

Attachment: part 1, introduce mozpresentation, v2

add helper function in nsContentUtils and remove docshell modifications.

Comment 26

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

Attachment: part 2, use presentation URL to identify presented content, v2

use the helper function in nsContentUtils, carry r+.

Comment 27

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

Attachment: part 3, update-test-cases.patch

rebase to m-c, carry r+.

Comment 28

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

Comment on attachment 8753170 [details] [diff] [review]
part 1, introduce mozpresentation, v2

>+nsContentUtils::GetPresentationURL(nsIDocShell* aDocShell, nsAString& aPresentationUrl)
>+{
>+  MOZ_ASSERT(aDocShell);
>+
>+  if (XRE_IsContentProcess()) {
>+    TabChild* tabChild = TabChild::GetFrom(aDocShell);
>+    aPresentationUrl = tabChild->PresentationURL();
>+    return;
>+  }
>+
>+  nsCOMPtr<nsILoadContext> loadContext(do_QueryInterface(aDocShell));
>+  nsCOMPtr<nsIDOMElement> topFrameElement;
>+  loadContext->GetTopFrameElement(getter_AddRefs(topFrameElement));

Just want to make sure...  if <iframe mozbrowser> used in an application, this will return the iframe, and not the container iframe of
the document which has that <iframe mozbrowser>. Is that expected?
Other option is to effectively inline GetTopFrameElement call, but use GetTop and not GetScriptableTop.
It is not clear to me which approach should be taken. I guess the current setup is fine. If some application using mozbrowser wants to expose presentation, 
it could first check whether it has presentation itself and if so, expose the attribute in the mozbrowser.

>+  nsAutoString presentationURLStr;
>+  if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozpresentation)) {
>+    mOwnerContent->GetAttr(kNameSpaceID_None,
>+                           nsGkAtoms::mozpresentation,
>+                           presentationURLStr);
>+  }
You don't need the HasAttr check.

Comment 29

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

(In reply to Olli Pettay [:smaug] from comment #28)
> Comment on attachment 8753170 [details] [diff] [review]
> part 1, introduce mozpresentation, v2
> 
> >+nsContentUtils::GetPresentationURL(nsIDocShell* aDocShell, nsAString& aPresentationUrl)
> >+{
> >+  MOZ_ASSERT(aDocShell);
> >+
> >+  if (XRE_IsContentProcess()) {
> >+    TabChild* tabChild = TabChild::GetFrom(aDocShell);
> >+    aPresentationUrl = tabChild->PresentationURL();
> >+    return;
> >+  }
> >+
> >+  nsCOMPtr<nsILoadContext> loadContext(do_QueryInterface(aDocShell));
> >+  nsCOMPtr<nsIDOMElement> topFrameElement;
> >+  loadContext->GetTopFrameElement(getter_AddRefs(topFrameElement));
> 
> Just want to make sure...  if <iframe mozbrowser> used in an application,
> this will return the iframe, and not the container iframe of
> the document which has that <iframe mozbrowser>. Is that expected?
> Other option is to effectively inline GetTopFrameElement call, but use
> GetTop and not GetScriptableTop.
> It is not clear to me which approach should be taken. I guess the current
> setup is fine. If some application using mozbrowser wants to expose
> presentation, 
> it could first check whether it has presentation itself and if so, expose
> the attribute in the mozbrowser.
Yes return the element of iframe is what I want. The <iframe mozbrowser> in side a presented content should not inherit the accessibility of presentation API. That's why I use |GetSameTypeRootTreeItem| in previous version.
This reminds me that the behavior for OOP browser element doesn't have the same behavior because I always check the TabChild but not the inner most <iframe mozbrowser>. If this scenario is something we should take into consider, this is the logic I'll apply:

if (oop) {
  if (docshell.rootTreeItem == docshell.sameTypeRootTreeItem) {
    return tabchild.presentatioURL();
  }
}

return docshell.topFrameElement.attribute["mozpresentation"];

Any comments?
> 
> >+  nsAutoString presentationURLStr;
> >+  if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozpresentation)) {
> >+    mOwnerContent->GetAttr(kNameSpaceID_None,
> >+                           nsGkAtoms::mozpresentation,
> >+                           presentationURLStr);
> >+  }
> You don't need the HasAttr check.
Will fix.

Comment 30

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

Attachment: part 1, introduce mozpresentation, v3

update according to comment #29.

Comment 31

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #29)
> if (oop) {
>   if (docshell.rootTreeItem == docshell.sameTypeRootTreeItem) {
>     return tabchild.presentatioURL();
>   }
> }
> 
> return docshell.topFrameElement.attribute["mozpresentation"];

That looks ok.

Comment 32

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

Attachment: part 1, introduce mozpresentation, v3

need a final check for nsContentUtils::GetPresentationURL.

Comment 33

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

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

Comment 34

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

Comment on attachment 8753799 [details] [diff] [review]
part 1, introduce mozpresentation, v3

>+  if (XRE_IsContentProcess()) {
>+    nsCOMPtr<nsIDocShellTreeItem> sameTypeRoot;
>+    aDocShell->GetSameTypeRootTreeItem(getter_AddRefs(sameTypeRoot));
>+    nsCOMPtr<nsIDocShellTreeItem> root;
>+    aDocShell->GetRootTreeItem(getter_AddRefs(root));
>+    if (sameTypeRoot.get() == root.get()) {
>+      // presentation URL is stored in TabChild for the top most
>+      // <iframe mozbrowser> in content process.
>+      TabChild* tabChild = TabChild::GetFrom(aDocShell);
>+      aPresentationUrl = tabChild->PresentationURL();
Actually, I wouldn't mind adding a null check for tabChild. Just return early if there is no tabChild.

Comment 35

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

Attachment: part 1, introduce mozpresentation

update according to review comments, rebase to m-c, carry r+

Comment 36

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

Attachment: part 2, use presentation URL to identify presented content

rebase to m-c, carry r+

Comment 37

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

Attachment: part 3, update-test-cases.patch

rebase to m-c, carry r+

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf300dda85a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a0848fb59ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/11db59507360

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android crashtest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42d3d42dafa

https://treeherder.mozilla.org/logviewer.html#?job_id=28239976&repo=mozilla-inbound#L2443

Comment 40

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

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

Comment 41

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

Attachment: part 2, use presentation URL to identify presented content

fix android crashtest issue

Comment 42

[Kershaw Chang [:kershaw]]

I've asked Gary to land your patches, since some of my works are depended on this bug.

However, there are conflicts when applying to inbound. Could you rebase your patch again, please?

Comment 43

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

Attachment: [inbound] p1.patch

Comment 44

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

Attachment: [inbound] p2.patch

Comment 45

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

Attachment: [inbound] p3.patch

Comment 46

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

I've update the patch for inbound, please help merge.
Thanks

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/660dc7b00368
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4b56d6e53c
https://hg.mozilla.org/integration/mozilla-inbound/rev/46845866285d

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/660dc7b00368
https://hg.mozilla.org/mozilla-central/rev/cd4b56d6e53c
https://hg.mozilla.org/mozilla-central/rev/46845866285d

Comment 49

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

Comment on attachment 8754181 [details] [diff] [review]
part 1, introduce mozpresentation

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 #): Presentation API
User impact if declined: presentation connection might fail
Testing completed: with automation test
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a

Comment 50

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

Comment on attachment 8754183 [details] [diff] [review]
part 3, update-test-cases.patch

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 #): Presentation API
User impact if declined: presentation connection might fail
Testing completed: with automation test
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a

Comment 51

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

Comment on attachment 8754618 [details] [diff] [review]
part 2, use presentation URL to identify presented content

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 #): Presentation API
User impact if declined: presentation connection might fail
Testing completed: with automation test
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: n/a

Comment 52

[Josh Cheng [:josh]]

Comment on attachment 8754181 [details] [diff] [review]
part 1, introduce mozpresentation

Approve for TV 2.6

Comment 53

[Josh Cheng [:josh]]

Comment on attachment 8754183 [details] [diff] [review]
part 3, update-test-cases.patch

Approve for TV 2.6

Comment 54

[Josh Cheng [:josh]]

Comment on attachment 8754618 [details] [diff] [review]
part 2, use presentation URL to identify presented content

Approve for TV 2.6

Comment 55

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

Attachment: rebase for tv 2.6

Comment 56

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/6b1d94f1edc5abc4f92953d685097a7333130364

Comment 57

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/3020a9629ea22bf70e8e3509b57d0cb7daf5a540

Comment 58

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

Attachment: rebase for tv 2.6, v2