If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[Presentation WebAPI] align the behavior of PresentationAvailability with latest spec

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: schien, Assigned: schien)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [ETA 9/2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

After bug 1194049 is landed, MDNS device provider is no long doing continuously device scan. Three approaches I have in mind:

1. Adding a timer and periodically perform force discovery if there is any PresentationAvailability created.
2. Introduce another XPIDL API in nsIPresentationDeviceManager for switching on continuously device scan.
3. Throw NotSupportedError exception at |getAvailability()| if there is any device provider not supporting continuously device scan.
Blocks: 1184073
The first thing to do is lazy initializing PresentationAvailability, therefore we can decide whether we should enter continuously scanning mode by monitoring the presence of AvailabilityListener in chrome process.
Created attachment 8692938 [details] [diff] [review]
[part 1] lazy-loading-availability.patch

1. lazy loading PresentationAvailability until |getAvailability()| is invoked.
2. PresentationDeviceManager is not available in content process, need to retrieve the current availability from chrome process.
Attachment #8692938 - Flags: review?(bugs)
Attachment #8692938 - Flags: feedback?(selin)
Attachment #8692938 - Flags: feedback?(selin) → feedback+
Blocks: 1227029

Comment 3

2 years ago
Comment on attachment 8692938 [details] [diff] [review]
[part 1] lazy-loading-availability.patch

Ok, so the patch changes PresentationRequest.getAvailability() quite a bit.
Before the patch one would get different Promise object each time, and with this patch it is always the same.

I think if .webidl had now 
readonly attribute Promise<PresentationAvailability> availability; 
the API would self-describe itself better.

But, the spec says GetAvailability() creates always a new Promise object. So the patch doesn't follow the spec.
So, either get the spec changed or fix the patch.
Attachment #8692938 - Flags: review?(bugs) → review-

Updated

a year ago
Whiteboard: [ETA 9/2]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8692938 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Summary: [Presentation WebAPI] change event for PresentationAvailability is not always fired for MDNS Device → [Presentation WebAPI] align the behavior of PresentationAvailability with latest spec
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Fix test failure and memory leakage, and rebase to latest m-c.

Comment 10

a year ago
mozreview-review
Comment on attachment 8781916 [details]
Bug 1228508 - Part 1, create new availability object for each getAvailability(). .

https://reviewboard.mozilla.org/r/72242/#review71098
Attachment #8781916 - Flags: review?(bugs) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8781917 [details]
Bug 1228508 - Part 2, maintain the set of availability objects.

https://reviewboard.mozilla.org/r/72244/#review71106

::: dom/presentation/PresentationRequest.cpp:366
(Diff revision 3)
>  
>    RefPtr<PresentationAvailability> availability =
> -    PresentationAvailability::Create(GetOwner(), promise);
> +    collection->Find(GetOwner()->WindowID(), mUrl);
>  
> -  if (!availability) {
> -    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +  if (availability) {
> +    aPromise->MaybeResolve(availability);

I don't understand this. Why do we want to resolve immediately? Spec seems to say the same, but don't we want to wait for actual result for the availability? Feels like a spec issue to me.

Or do I misunderstand this?

Comment 12

a year ago
mozreview-review
Comment on attachment 8781917 [details]
Bug 1228508 - Part 2, maintain the set of availability objects.

https://reviewboard.mozilla.org/r/72244/#review71110

So I think the spec is buggy.
Or, I am misreading it, and if so, please explain and ask review again.
Attachment #8781917 - Flags: review?(bugs) → review-
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8781917 [details]
Bug 1228508 - Part 2, maintain the set of availability objects.

https://reviewboard.mozilla.org/r/72244/#review71106

> I don't understand this. Why do we want to resolve immediately? Spec seems to say the same, but don't we want to wait for actual result for the availability? Feels like a spec issue to me.
> 
> Or do I misunderstand this?

From my understanding, the availability object we found in AvailabilityCollection already cache the latest result of device availability. Therefore we can simply return the same availability object because browser is already monitoring the available device for designated presentation URL and will keep update the status to that object.
see my explanation in comment #13.
Flags: needinfo?(bugs)
And then one needs to add change event listener to the availability object?

So, in first time getAvailability() is called, the promise is resolved once we know something is available, but the next time it is called, promise is resolved immediately?

In other words, what does the following print if resource is available:
request.getAvailability().then(function() {console.log(1)});
request.getAvailability().then(function() {console.log(2)});


I think per spec it is 2, 1, but IMO 1, 2 would be less confusing.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #15)
> And then one needs to add change event listener to the availability object?
> 
> So, in first time getAvailability() is called, the promise is resolved once
> we know something is available, but the next time it is called, promise is
> resolved immediately?
> 
> In other words, what does the following print if resource is available:
> request.getAvailability().then(function() {console.log(1)});
> request.getAvailability().then(function() {console.log(2)});
> 
> 
> I think per spec it is 2, 1, but IMO 1, 2 would be less confusing.

In step #10 in spec, the promise is resolved without waiting for device available. So, the output of your sample code should be 1, 2. I'll double check if my current implementation matched with it.

I can file a spec bug to explicitly state that step #9 is running asynchronously in background. How do you think?
Flags: needinfo?(bugs)
Right. That is the case I'm talking about. I read the spec so that #9 may take long time so #10 is possibly postponed. And since everything after #3 is run parallel, nothing guarantees the next call to getAvailability() doesn't return and resolve the promise before the first call.

But if #9 is run async, why the use of Promise here when one anyway needs to wait for the change event?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #17)
> Right. That is the case I'm talking about. I read the spec so that #9 may
> take long time so #10 is possibly postponed. And since everything after #3
> is run parallel, nothing guarantees the next call to getAvailability()
> doesn't return and resolve the promise before the first call.
> 
> But if #9 is run async, why the use of Promise here when one anyway needs to
> wait for the change event?
If browser happens to have some cached available device record, the PresentationAvailability.value will be set to true initially as step #7 described. There will be no change event fired even if step #9 is run asynchronously.
Flags: needinfo?(bugs)
Well, at that point we do know there is something available and could just return PresentationAvailability with value true.
I'm not against using Promises here, so either way.


But anyhow, I think the main issue is that nothing says #9 runs parallel to #10, so per spec 
request.getAvailability().then(function() {console.log(1)});
request.getAvailability().then(function() {console.log(2)});
should print 2, 1.
Could you file the spec bug about making #9 async
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (vacation Aug 25-28) from comment #19)
> Well, at that point we do know there is something available and could just
> return PresentationAvailability with value true.
> I'm not against using Promises here, so either way.
> 
> 
> But anyhow, I think the main issue is that nothing says #9 runs parallel to
> #10, so per spec 
> request.getAvailability().then(function() {console.log(1)});
> request.getAvailability().then(function() {console.log(2)});
> should print 2, 1.
> Could you file the spec bug about making #9 async

spec bug is filed: https://github.com/w3c/presentation-api/issues/335

I also raise one additional issue in the same bug that we can only return the same availability object on the same window
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8781916 - Flags: review+ → review?(bugs)

Comment 23

a year ago
mozreview-review
Comment on attachment 8781916 [details]
Bug 1228508 - Part 1, create new availability object for each getAvailability(). .

https://reviewboard.mozilla.org/r/72242/#review73172

::: dom/presentation/PresentationService.cpp:619
(Diff revisions 2 - 3)
>    if (NS_WARN_IF(!deviceManager)) {
>      return aCallback->NotifyError(NS_ERROR_DOM_OPERATION_ERR);
>    }
>  
> +  nsCOMPtr<nsIMutableArray> presentationUrls
> +    = do_CreateInstance(NS_ARRAY_CONTRACTID);

nit, I would put = to the end of the previous line.
Attachment #8781916 - Flags: review?(bugs) → review+

Comment 24

a year ago
mozreview-review
Comment on attachment 8781917 [details]
Bug 1228508 - Part 2, maintain the set of availability objects.

https://reviewboard.mozilla.org/r/72244/#review73178

::: dom/presentation/PresentationAvailability.cpp:173
(Diff revisions 3 - 4)
>  
>    mIsAvailable = aIsAvailable;
>  
> -  if (mPromise) {
> +  if (!mPromises.IsEmpty()) {
>      // Use the first availability change notification to resolve promise.
> -    mPromise->MaybeResolve(this);
> +    for (auto promise = mPromises.begin(); promise != mPromises.end(); promise++) {

This looks a bit scary. I couldn't prove that MaybeResolve() doesn't ever run scripts synchronously, and if scripts can be run sync, iteration can be totally bogus if one ends up adding more items to the array.


So, please have
nsTArray<RefPtr<Promise>> promises = Move(mPromises);
and then iterate promises and not mPromises.
And then no need to explicitly clear mPromises array.
That would at least be safe, even if behavior might be a bit odd if more promises were added.
But that shouldn't happen in any normal case.


In general array iteration using .begin()/.end() or ranged loop is security hazard unless one can prove that the array size can't change during the loop.
Attachment #8781917 - Flags: review?(bugs) → review+
If we want to ensure we handle all the promises we'd need to do something like

while (!mPromises.IsEmpty()) {
  nsTArray<RefPtr<Promise>> promises = Move(mPromises);
  for (auto promise = promises.begin(); promise != promises.end(); promise++) {
    promise->MaybeResolve(this);
  }
  // more promises may have been added to mPromises, at least in theory
}
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Olli Pettay [:smaug] from comment #25)
> If we want to ensure we handle all the promises we'd need to do something
> like
> 
> while (!mPromises.IsEmpty()) {
>   nsTArray<RefPtr<Promise>> promises = Move(mPromises);
>   for (auto promise = promises.begin(); promise != promises.end();
> promise++) {
>     promise->MaybeResolve(this);
>   }
>   // more promises may have been added to mPromises, at least in theory
> }

Thanks for the detailed review comment. Here is the result after addressing all the review comments:

In following sample code:
>var req = new PresentationRequest(requestUrl);
>req.getAvailability().then(function() {
>  console.log('1');
>  req.getAvailability().then(function() { console.log('1-1'); });
>  req.getAvailability().then(function() { console.log('1-2'); });
>}).then(req.getAvailability()).then(function() { console.log('1.1'); });
>req.getAvailability().then(function() {
>  console.log('2');
>});

The console output is
>1
>2
>1-1
>1-2
>1.1

This matched with the execution order of |getAvailability|.

Comment 29

a year ago
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ca07578f0c64
Part 1, create new availability object for each getAvailability(). r=smaug.
https://hg.mozilla.org/integration/autoland/rev/e413ced9ff12
Part 2, maintain the set of availability objects. r=smaug

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ca07578f0c64
https://hg.mozilla.org/mozilla-central/rev/e413ced9ff12
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.