Closed Bug 1228508 Opened 9 years ago Closed 8 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: schien, Assigned: schien)

References

Details

(Whiteboard: [ETA 9/2])

Attachments

(2 files, 1 obsolete file)

Bug 1228508 - Part 1, create new availability object for each getAvailability(). .
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1228508 - Part 2, maintain the set of availability objects.
58 bytes, text/x-review-board-request
smaug
: review+
Details 
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.
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.
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+
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-
Whiteboard: [ETA 9/2]
Attachment #8692938 - Attachment is obsolete: true
Summary: [Presentation WebAPI] change event for PresentationAvailability is not always fired for MDNS Device → [Presentation WebAPI] align the behavior of PresentationAvailability with latest spec
Fix test failure and memory leakage, and rebase to latest m-c.
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 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 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-
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
Attachment #8781916 - Flags: review+ → review?(bugs)
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 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
}
(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|.
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
https://hg.mozilla.org/mozilla-central/rev/ca07578f0c64
https://hg.mozilla.org/mozilla-central/rev/e413ced9ff12
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: