Add code to allocate agent clusters. #4617

dtapuska · 2019-05-13T19:54:23Z

Store shared agent clusters in the browsing context group via a map of
scheme & registrable domain.

Hook the document initialization methods to obtain a similar-orgin
window agent.

/browsers.html ( diff )
/browsing-the-web.html ( diff )
/infrastructure.html ( diff )
/webappapis.html ( diff )

dtapuska · 2019-05-13T19:57:52Z

@annevk @domenic Here is a crack at the allocating agent clusters. It closely matches the allocation that we have planned for Chrome.

I've made a change to what Anne provided as a rough sketch around naming the items shared agent clusters. Because I presume there will be agent clusters that will exist but not be shared, and they wouldn't be in this list. For example I'd expect that we could implement the feature policy in these steps and just return early providing a new agent cluster for everyone that has the policy set.

annevk

In general I like this approach, except that I think we need to refactor navigate so that documents are not the source of truth, but only an end result. I'd rather not further enshrine the weird "document's JavaScript API comes alive later" model.

source

annevk · 2019-05-14T10:12:01Z

source

@@ -89103,6 +89115,66 @@ import "https://example.com/foo/../module2.mjs";</code></pre>
  in particular among which <span data-x="agent">agents</span> the backing data of
  <code>SharedArrayBuffer</code> objects can be shared.</p>

+  <p>A <span>browsing context group</span> has associated <dfn data-export="">shared agent
+  clusters</dfn> (a <span data-x="ordered map">map</span> of <span>agent cluster key</span> to
+  <span>agent cluster</span>).</p>


Do we need to export "shared agent clusters"? Also, if you have a different strategy in mind, it seems we don't need the agent cluster key abstraction?

Removed the export. I do think the definition of the agent cluser key makes it clear it is based on the registrable domain.

source

domenic · 2019-05-14T13:49:51Z

Does it seem feasible to do this incrementally, by first fixing agent clusters, then fixing the ordering of Document allocation? Tying them together seems to increase the likelihood that only you will be able to ultimately take this on...

littledan · 2019-05-14T14:36:04Z

It's great to see this logic written out explicitly. Such an approach might also help us to be more explicit on the JS side, so cc @syg @lars-t-hansen who wrote much of that spec text.

annevk · 2019-05-15T13:31:37Z

I think I can live with document being created first and letting the bugs around that stay with us a little longer. However, we should get this to a stage whereby we no longer have to define the declarative setup around similar-origin window agents (i.e., can remove "same-agent Window objects") and whereby allocation of a new realm happens in the context of a particular agent such that it's appended to the agent's set of realms. (The current PR appears to do this in the wrong order, too.)

If we mark #4361 as fixed we should also file a follow-up for workers, though maybe there's something similar already tracking that.

dtapuska · 2019-05-23T15:11:31Z

@annevk Ok, I've adjusted my approach. I've pulled the determination of the sandbox flags and the origin before the document is created. This allows me to create the agent before the realm is created and pass the agent into that. Make sense?

annevk · 2019-05-24T12:48:52Z

Unfortunately, I'm having a hard time trying to understand this patch. E.g., determining the origin only really works when you're creating a popup or some such. Otherwise, when there's no creator origin, you're still highly dependent on existing infrastructure, right? I also miss the aspects I hinted at in my previous comment, i.e., why do we now have both new imperative-style and old declarative-style allocation of agents and agent clusters? We should be able to remove some of that, no? (Not the worker parts yet, but some of the window parts.)

dtapuska · 2019-07-12T13:25:03Z

@annevk I've rebased this on the navigation algorithm now. Can you take a look at it now?

annevk

I haven't really verified if all the things happen in the right order and whether all callsites are modified appropriately.

source

annevk · 2019-07-15T08:38:33Z

I think you forgot to push.

dtapuska · 2019-07-15T16:20:47Z

I think you forgot to push.

It wasn't quite ready when I left Friday. I perhaps am using git incorrectly because I'd really like to respond to comments all at once. I've been able to do that on a review but not responding to individual feedback. Anyways I've pushed the latest changes now.

domenic · 2019-07-15T16:30:05Z

It's a bit unintuitive, but you can submit your own "review" of type "comment" (as opposed to "approved" or "changes requested" types) and that allows drafting and batch-submission of replies.

annevk

By and large this is correct, but there's a number of important edge cases we need to get right. Some we might have to defer though as TC39 hasn't really resolved the issues on their end.

source

annevk · 2019-07-17T13:03:47Z

source

-   false.</p></li>
+  <p class="note">Two <code>Window</code> objects that have the same <span>agent</span> does
+  not indicate they can share data with each other, only that it is possible. Two windows
+  might only share data if they are <span>same origin-domain</span>.</p>


I think this sentence is wrong on multiple grounds due to postMessage(). And if you make it synchronous access it's still somewhat suspect due to SharedArrayBuffer I think?

Maybe say something about not necessarily being able to directly access all objects created in such a Window object's realm?

I've adjusted this and put a link to IsPlatformObjectSameOrigin

What does "might be" mean?

annevk · 2019-07-17T13:04:53Z

source

@@ -90393,6 +90357,68 @@ import "https://example.com/foo/../module2.mjs";</code></pre>
  in particular among which <span data-x="agent">agents</span> the backing data of
  <code>SharedArrayBuffer</code> objects can be shared.</p>

+  <p>A <span>browsing context group</span> has associated <dfn>agent cluster map</dfn> (a <span
+  data-x="ordered map">map</span> of <span>agent cluster key</span> to <span>agent
+  cluster</span>).</p>


We need to clarify here that we only hold these agent clusters weakly, i.e., user agents are responsible for collecting them when it is deemed that nothing can access them anymore.

source

annevk · 2019-07-19T11:35:40Z

source

     <li>
      <p>Let <var>realm execution context</var> be the result of <span>creating a new JavaScript
-      realm</span> with the following customizations:</p>
+      realm</span> in <var>agent</var> with the following customizations:</p>


It seems you haven't modified this similarly to how @domenic suggested above.

dtapuska

PTAL, I think I've addressed all the open issues. I've fixed merge conflicts via a forced push.

source

dtapuska · 2019-07-29T15:02:36Z

source

     <li>
      <p>Let <var>realm execution context</var> be the result of <span>creating a new JavaScript
-      realm</span> with the following customizations:</p>
+      realm</span> in <var>agent</var> with the following customizations:</p>


annevk

Thanks, I only have a number of nits left. @domenic should probably also have a final look.

annevk · 2019-08-13T13:49:56Z

source

-   <span>relevant settings object</span>'s <span
-   data-x="concept-settings-object-origin">origin</span>.</p></li>
+  <p class="note">Two <code>Window</code> objects that have the same <span>agent</span> does not
+  indicate they can directly access all objects created in each others realms. Two windows might be


Should there not be an apostrophe after "others"?

I believe it should be "each other's". Changed.

annevk · 2019-08-13T13:50:56Z

source

-   <span>relevant settings object</span>'s <span
-   data-x="concept-settings-object-origin">origin</span>.</p></li>
+  <p class="note">Two <code>Window</code> objects that have the same <span>agent</span> does not
+  indicate they can directly access all objects created in each others realms. Two windows might be


Instead of "Two windows" maybe use "They" as otherwise it's rather informal and not directly clear objects are meant.

annevk · 2019-08-13T13:51:26Z

source

-   data-x="concept-settings-object-origin">origin</span>.</p></li>
+  <p class="note">Two <code>Window</code> objects that have the same <span>agent</span> does not
+  indicate they can directly access all objects created in each others realms. Two windows might be
+  required to be in <span>same origin-domain</span>, see <span>IsPlatformObjectSameOrigin</span>.</p>


We cannot use "required" in a note. Maybe say "They would have to be same origin-domain, see ..."

source

domenic

I got really confused before I realized that yes, there is only one similar-origin window agent per agent cluster. There can be more stuff, but it's all non-window agents. So I added one commit, on top of my editorial ones, that tries to make the situation clearer. Please take a look and let me know if I'm on the right track.

Otherwise, I have one TODO about allocating the agent and defining "similar-origin window agent" more correctly.

domenic · 2019-08-14T16:57:50Z

source

@@ -90397,6 +90374,70 @@ import "https://example.com/foo/../module2.mjs";</code></pre>
  in particular among which <span data-x="agent">agents</span> the backing data of
  <code>SharedArrayBuffer</code> objects can be shared.</p>

+  <p>A <dfn data-export="">scheme-and-site</dfn> is a <span>tuple</span> of a <span


Is it just me, or would it make more sense to call it scheme-and-domain??

I called it site due to https://url.spec.whatwg.org/#host-same-site. "scheme-and-registrable-domain" might work, but "domain" seems too ambiguous.

domenic · 2019-08-14T17:02:31Z

source

+   data-x="obtain-browsing-agent-cluster">obtaining a browsing context agent cluster</span> with
+   <var>group</var> and <var>clusterKey</var>.</p></li>
+
+   <li><p>Return <var>agentCluster</var>'s <span>similar-origin window agent</span>.</p></li>


This links to the definition of the concept of similar-origin window agent. But it's used as a field on agentCluster.

I think the intention here is something like "return the single agent contained in agentCluster". (The ES spec seems to define agent clusters to be be "sets of agents".) Similarly below, in "obtain a browsing context agent cluster".

agentCluster would have multiple agents. It would only have a single window agent though, the other agents will be for workers or worklets. I was trying to phrase something like "return agentCluster's single window agent".

domenic · 2019-08-14T17:09:08Z

source

+  </ol>
+
+  <p>A <dfn data-export="">similar-origin window agent</dfn> is an <span>agent</span> whose
+  [[CanBlock]] is false. </p>


This doesn't seem like a sufficient definition; in particular there are others with [[CanBlock]] of false. I think instead you want something like "A similar-origin window agent is an agent created during the course of the following algorithm:" and then inside the algorithm you create a new "agent".

That new agent should get all its fields set. In particular:

[[CanBlock]] to false

[[Signifier]] to ... a new unique value?

[[CandidateExecution]] to a new empty candidate execution

[[LittleEndian]], [[IsLockFree1]], and [[IsLockFree2]] at the implement's discretion.

Thus far we only defined values that are different given that ECMAScript doesn't define concrete agent allocation. So I think continuing with only specifying [[CanBlock]] is fine, but we should probably use "new" in there.

I'm not sure what ECMAScript really needs to define. It's just a record with a bunch of fields; you can create those pretty easily.

Domenic I've written what you suggested. Anne I tried to add "new" in there and it felt that Domenic's proposal was more natural.

annevk

Found more nits. I might be able to push this over the finish line once we're all aligned.

source

annevk · 2019-08-15T13:12:25Z

source

+  </ol>
+
+  <p>A <dfn data-export="">similar-origin window agent</dfn> is an <span>agent</span> whose
+  [[CanBlock]] is false. </p>


Thus far we only defined values that are different given that ECMAScript doesn't define concrete agent allocation. So I think continuing with only specifying [[CanBlock]] is fine, but we should probably use "new" in there.

source

Store shared agent clusters in the browsing context group via a map of scheme & registrable domain. Hook the document initialization methods to obtain a similar-orgin window agent. Fixes whatwg#4361

- Remove extra comments - Add period - Remove useless text - Add formalism around creation of Window Agent

- Remove window agent definition - shared agent clusters -> agent cluster map

Biggest thing is relocating the definition of "similar-origin window agent" to the agents section instead of agent cluster section

domenic

OK, looks good to me!! @dtapuska can you sanity-check my latest changes? I'm ready to pull this in now if you are.

dtapuska

I'm good with this

annevk reviewed May 14, 2019

View reviewed changes

dtapuska force-pushed the agent_cluster branch from ce46cfb to c8bb7d8 Compare May 23, 2019 15:09

dtapuska force-pushed the agent_cluster branch 2 times, most recently from d3bf0a4 to fde5072 Compare May 27, 2019 14:55

dtapuska force-pushed the agent_cluster branch from fde5072 to 2b052d4 Compare July 11, 2019 16:01

annevk reviewed Jul 12, 2019

View reviewed changes

annevk reviewed Jul 16, 2019

View reviewed changes

annevk mentioned this pull request Jul 17, 2019

User agents, session history, browsing contexts, and agent clusters #4782

Closed

annevk reviewed Jul 17, 2019

View reviewed changes

annevk reviewed Jul 19, 2019

View reviewed changes

dtapuska force-pushed the agent_cluster branch from e544978 to 7d683e7 Compare July 29, 2019 15:17

dtapuska commented Jul 29, 2019

View reviewed changes

dtapuska mentioned this pull request Jul 29, 2019

Add disallowdocumentaccess iframe attribute to force a creation of a new agent cluster map #4606

Open

3 tasks

annevk reviewed Aug 13, 2019

View reviewed changes

dtapuska force-pushed the agent_cluster branch from 7d683e7 to 73622c5 Compare August 13, 2019 15:14

domenic force-pushed the agent_cluster branch from 691f4ce to 9aeb482 Compare August 14, 2019 17:33

domenic requested changes Aug 14, 2019

View reviewed changes

annevk reviewed Aug 15, 2019

View reviewed changes

dtapuska and others added 2 commits August 28, 2019 14:02

Add code to allocate agent clusters.

f1467f9

Store shared agent clusters in the browsing context group via a map of scheme & registrable domain. Hook the document initialization methods to obtain a similar-orgin window agent. Fixes whatwg#4361

Remove export of definition

2f9f811

dtapuska and others added 9 commits August 28, 2019 14:02

- Add note

3cbcf82

- Remove extra comments - Add period - Remove useless text - Add formalism around creation of Window Agent

Fix a few things

6c1dff4

- Remove window agent definition - shared agent clusters -> agent cluster map

More fixes

3e2b1ed

Fix passing of agent into Javascript realm.

4a84c14

Remove "in agent" and fix one missed section

fdca096

Fix nits

87da2da

Editorial tweaks

be3aef0

More substantial elaborations

fe00836

Add fixes to latest feedback

5932a85

dtapuska force-pushed the agent_cluster branch from 9aeb482 to 5932a85 Compare August 28, 2019 18:54

Nits and tweaks

7419758

Biggest thing is relocating the definition of "similar-origin window agent" to the agents section instead of agent cluster section

domenic approved these changes Sep 9, 2019

View reviewed changes

domenic mentioned this pull request Sep 9, 2019

Imperative allocation of agent clusters #4361

Closed

dtapuska commented Sep 9, 2019

View reviewed changes

domenic merged commit 0404472 into whatwg:master Sep 9, 2019

domenic added the topic: agent The interaction with JavaScript's agent and agent cluster concepts label Sep 9, 2019

This was referenced Apr 13, 2020

HTML Standard (Jan 2020): review tracker w3c/htmlwg#8

Open

Review request: HTML Standard Jan 2020 Review Draft w3ctag/design-reviews#499

Closed

hober mentioned this pull request Apr 13, 2020

HTML Horizontal Review: Windows, navigation, agents and agent clusters w3ctag/design-reviews#502

Closed

3 tasks

Add code to allocate agent clusters. #4617

Add code to allocate agent clusters. #4617

Conversation

dtapuska commented May 13, 2019 • edited by pr-preview bot Loading

dtapuska commented May 13, 2019

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented May 14, 2019

littledan commented May 14, 2019

annevk commented May 15, 2019

dtapuska commented May 23, 2019

annevk commented May 24, 2019

dtapuska commented Jul 12, 2019

annevk left a comment

Choose a reason for hiding this comment

annevk commented Jul 15, 2019

dtapuska commented Jul 15, 2019

domenic commented Jul 15, 2019

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtapuska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

dtapuska left a comment

Choose a reason for hiding this comment

dtapuska commented May 13, 2019 •

edited by pr-preview bot

Loading

domenic left a comment •

edited

Loading