Closed Bug 1234128 Opened 4 years ago Closed 4 years ago

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

Categories

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

Unspecified
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
blocking-b2g 2.6+
Tracking Status
firefox49 --- fixed
b2g-v2.6 --- fixed

People

(Reporter: kuoe0.tw, Assigned: schien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ETA 5/26])

Attachments

(7 files, 13 obsolete files)

14.37 KB, patch
schien
: review+
Details | Diff | Splinter Review
38.70 KB, patch
schien
: review+
Details | Diff | Splinter Review
11.15 KB, patch
schien
: review+
Details | Diff | Splinter Review
14.48 KB, patch
Details | Diff | Splinter Review
11.15 KB, patch
Details | Diff | Splinter Review
38.70 KB, patch
Details | Diff | Splinter Review
48 bytes, text/x-github-pull-request
Details | Review
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.
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.
Summary: navigator.presentation.receiver is null in 1-UA use case. → [Presentation WebAPI] navigator.presentation.receiver is null in 1-UA use case.
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.
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.
Attachment #8731565 - Flags: review?(schien)
Attachment #8731565 - Flags: review?(kuoe0)
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.
Attachment #8731565 - Flags: review?(schien) → review-
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
Flags: needinfo?(schien)
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
Flags: needinfo?(schien)
(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. )
(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.
Assignee: nobody → schien
blocking-b2g: --- → 2.6?
blocking-b2g: 2.6? → 2.6+
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.
WIP for marking presented content with "mozPresentation" attribute.

TODOs
- support non-remote iframe
- check same-origin policy for iframe inside the presented content
Attachment #8731565 - Attachment is obsolete: true
Attachment #8731565 - Flags: review?(kuoe0)
Whiteboard: [ETA 5/26]
Introduce 'mozpresentation' attribute for browser element for identifying the scope of presented content.
Attachment #8746352 - Attachment is obsolete: true
Attachment #8749148 - Flags: review?(khuey)
add "mozpresentation=<requested URL>" in browser element.
Attachment #8749148 - Attachment is obsolete: true
Attachment #8749148 - Flags: review?(khuey)
Attachment #8750169 - Flags: review?(bugs)
use the presentation URL stored in nsDocShell to identify the window for presented content.
Attachment #8750170 - Flags: review?(bugs)
Attached patch part 3, test cases (obsolete) — Splinter Review
update test cases to use mozbrowser and mozpresentation.
Attachment #8750171 - Flags: review?(bugs)
Reviews are coming. Sorry about delay. Trying to get to this and other presentation API patches tomorrow.
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 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
Attachment #8750169 - Flags: review?(bugs) → review-
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.
Attachment #8750170 - Flags: review?(bugs) → review+
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+
Attachment #8750171 - Flags: review?(bugs) → review+
(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.
I see. ok, thanks.
add helper function in nsContentUtils and remove docshell modifications.
Attachment #8750169 - Attachment is obsolete: true
Attachment #8753170 - Flags: review?(bugs)
use the helper function in nsContentUtils, carry r+.
Attachment #8750170 - Attachment is obsolete: true
Attachment #8753171 - Flags: review+
Attached patch part 3, update-test-cases.patch (obsolete) — Splinter Review
rebase to m-c, carry r+.
Attachment #8750171 - Attachment is obsolete: true
Attachment #8753172 - Flags: review+
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.
Attachment #8753170 - Flags: review?(bugs) → review+
(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.
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs)
Attachment #8753170 - Attachment is obsolete: true
need a final check for nsContentUtils::GetPresentationURL.
Attachment #8753680 - Attachment is obsolete: true
Attachment #8753799 - Flags: review?(bugs)
Attachment #8753172 - Attachment description: update-test-cases.patch → part 3, update-test-cases.patch
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.
Attachment #8753799 - Flags: review?(bugs) → review+
update according to review comments, rebase to m-c, carry r+
Attachment #8753799 - Attachment is obsolete: true
Attachment #8754181 - Flags: review+
rebase to m-c, carry r+
Attachment #8753171 - Attachment is obsolete: true
Attachment #8754182 - Flags: review+
rebase to m-c, carry r+
Attachment #8753172 - Attachment is obsolete: true
Attachment #8754183 - Flags: review+
fix android crashtest issue
Attachment #8754182 - Attachment is obsolete: true
Attachment #8754618 - Flags: review+
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?
Flags: needinfo?(schien)
Flags: needinfo?(schien)
I've update the patch for inbound, please help merge.
Thanks
Flags: needinfo?(xeonchen)
Flags: needinfo?(xeonchen)
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
Attachment #8754181 - Flags: approval-mozilla-b2g48?
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
Attachment #8754183 - Flags: approval-mozilla-b2g48?
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
Attachment #8754618 - Flags: approval-mozilla-b2g48?
Comment on attachment 8754181 [details] [diff] [review]
part 1, introduce mozpresentation

Approve for TV 2.6
Attachment #8754181 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Comment on attachment 8754183 [details] [diff] [review]
part 3, update-test-cases.patch

Approve for TV 2.6
Attachment #8754183 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Comment on attachment 8754618 [details] [diff] [review]
part 2, use presentation URL to identify presented content

Approve for TV 2.6
Attachment #8754618 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Attached file rebase for tv 2.6 (obsolete) —
Flags: needinfo?(xeonchen)
Attached file rebase for tv 2.6, v2
Attachment #8758191 - Attachment is obsolete: true
Flags: needinfo?(xeonchen)
Flags: needinfo?(xeonchen)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.