Skip to content

Issue 387: Need to make sure we have a display list for start(). #390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

markafoltz
Copy link
Contributor

Addresses issue #387: Filling out the list of available presentation displays for start.

If there is no existing presentation display availability for the request, creates a temporary one and runs the steps to monitor. The temporary one is removed at the end of start().

@tidoust
Copy link
Member

tidoust commented Dec 5, 2016

I see why cleaning up the temporary presentation availability display could be useful but, while stupid, cannot an app call getAvailability() immediately after calling start() and before the selection algorithm is over? start() should not reset presentation display availability when it ends in that case because getAvailability needs it.

This triggers another issue, actually: when can the presentation display availability be garbage collected? I read the current getAvailability algorithm as implying that the presentation display availability (and the presentation availability promise) are set once and for all for the entire duration of the browsing context, and thus won't ever be garbage collected.

In turn, this would mean that the garbage collection steps envisioned in the monitoring section will never run: "When a PresentationAvailability object is no longer alive (i.e., is eligible for garbage collection), the user agent SHOULD [...]"... because the availability objects would always remain "alive".

Am I reading this correctly? If so, the above GC steps need fixing. If not, I think the spec needs to be clearer that the presentation display availability and the presentation availability promise may be collected at any time (well, the promise must be collected before the availability object, since it contains a pointer to it).

@markafoltz
Copy link
Contributor Author

I see why cleaning up the temporary presentation availability display could be useful but, while stupid, cannot an app call getAvailability() immediately after calling start() and before the selection algorithm is over? start() should not reset presentation display availability when it ends in that case because getAvailability needs it.

Is it possible for the getAvailability() steps to occur during the start() steps and before the Cleanup step?

If so, we could have getAvailability() reject if there is an unsettled Promise from start(), or add a flag to indicate when the temporary availability object may cleaned up.

This triggers another issue, actually: when can the presentation display availability be garbage collected? I read the current getAvailability algorithm as implying that the presentation display availability (and the presentation availability promise) are set once and for all for the entire duration of the browsing context, and thus won't ever be garbage collected.

Association of an availability object with a request shouldn't create a strong reference for the purposes of GC. It's to prevent creation of duplicate objects. How do specs label kinds of ownership of objects for GC purposes?

Am I reading this correctly? If so, the above GC steps need fixing. If not, I think the spec needs to be clearer that the presentation display availability and the presentation availability promise may be collected at any time (well, the promise must be collected before the availability object, since it contains a pointer to it).

They should be collected like any other object, when they go out of scope for the associated script context's scope. We should be more specific about the relationships among these objects so GC behavior is clear.

@tidoust
Copy link
Member

tidoust commented Dec 6, 2016

Is it possible for the getAvailability() steps to occur during the start() steps and before the Cleanup step?

It seems possible at least given that steps run in parallel each time.

If so, we could have getAvailability() reject if there is an unsettled Promise from start()

It seems strange to tie the result of getAvailability() to start().

or add a flag to indicate when the temporary availability object may cleaned up.

Right, that would solve the problem.

Association of an availability object with a request shouldn't create a strong reference for the purposes of GC. It's to prevent creation of duplicate objects. How do specs label kinds of ownership of objects for GC purposes?

I don't remember having seen strong/weak terminology used in other specs but maybe I missed something. It is not unusual to see garbage collection sections that describe when objects can be collected though. I do not know if there is an implicit default that the "internal slots" that a spec may use are weak references. It seems a good idea to be explicit about the intent in any case.

They should be collected like any other object, when they go out of scope for the associated script context's scope. We should be more specific about the relationships among these objects so GC behavior is clear.

OK, then below is how I would fix this issue and clarify GC behavior. Happy to craft a PR if you think that's a good approach.

In start()

Keep the algorithm as it exists today.

If you'd rather continue in the direction you set in this PR, I think you need to "synchronize" the start and getAvailability steps that set the presentation display availability and fill out the set of presentation availability objects, otherwise you could in theory end up with duplicate entries in the set. It should be doable by shuffling things a bit to move these steps before the algorithms return the promise (and thus before algorithms start to run in parallel).

No clean up step should be needed.

In getAvailability()

I think the intent with getAvailability is to return the same Promise as long as it is unsettled. To be confirmed with @schien (also raised in #388 (comment)). If the goal is rather to return the same Promise as long as the code holds a reference to a Promise previously returned by that function, the suggestion below does not work.

I propose to get rid of the notion of presentation availability promise entirely not to have to worry about specifying GC conditions for it:

  1. replace current step 2 with: "If there is an unsettled Promise from a previous call to getAvailability on presentationRequest, return that Promise and abort these steps". This would make the method return the same Promise as long as it is unsettled.
  2. adjust steps 3 and 4 accordingly

In the monitoring algorithm

Start with a couple of steps that clone and extend the set of presentation availability objects if needed:

  1. Let availabilitySet be a shallow clone of the set of presentation availability objects
  2. If there is a pending request to "select a presentation display" for a presentation request and if the presentation display availability of that presentation request is null, then create a tuple (A, presentationUrls) where presentationUrls are the presentation request URLs and add it to availabilitySet

Then use availabilitySet in the rest of the algorithm.

An alternative approach would be to craft a separate set of steps for the pending request to "select a presentation display" as suggested in a previous comment (#387 (comment))

Also replace "When a PresentationAvailability object is no longer alive" with "When a presentation display availability object is no longer alive"

Next to the definition of presentation display availability

Add a sentence to say that user agents MAY collect the presentation display availability for a PresentationRequest at any time when the object goes out of scope for the associated script context's scope.

@markafoltz
Copy link
Contributor Author

Overall I like the approach you propose, here are a few things I noted. Go ahead with a PR when you are ready.

Keep the algorithm as it exists today.

Sounds good.

I propose to get rid of the notion of presentation availability promise entirely not to have to worry about specifying GC conditions for it:

That would simplify things; but Web developers would need to make sure to expunge references to any Promises as well as the resulting availability object, in order for the availability object to be garbage collected. This is a valid idiom for Promise-vending APIs, and I don't object to it.

Start with a couple of steps that clone and extend the set of presentation availability objects if needed:

Since the monitoring steps run in parallel, if getAvailability() is called while it is running, we would just want to make sure that any new availability object created by those steps is inserted into the original set of presentation availability objects and not the clone; and that the monitoring steps are run again to pick up the new object.

when the object goes out of scope for the associated script context's scope.

This is how Web Bluetooth puts it:

When no ECMAScript code can observe an instance of BluetoothRemoteGATTServer server anymore, the UA SHOULD run server.disconnect().

Or we could just say "is eligible for garbage collection?"

It gets tricky because a script context can have multiple Realms / settings objects - so we would need to be specific about which one applies. See Issue #391 I filed.

@markafoltz
Copy link
Contributor Author

This PR is obsoleted by PR #387.

@markafoltz markafoltz closed this Dec 22, 2016
@markafoltz markafoltz deleted the issue-387-start-availability branch December 22, 2016 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants