camillelamy / explainers

11 stars 5 forks source link

WindowProxy design looks really complicated #7

Open annevk opened 4 years ago

annevk commented 4 years ago

I'm a little worried about this design spanning the entire browsing context tree. I recall discussing a much simpler version with @arturjanc.

And I think it was very similar to navigations. "Someone is accessing me" and "I'm accessing someone else". What motivated the design change?

camillelamy commented 4 years ago

I think the idea is still the same, it's just that when we lay out the details of how to do it it becomes complicated. Basically, identifying "someone is accessing and it's a problem" from "someone is accessing me and it is not a problem" requires a lot of bookkeeping.

I am not completely sure if I understand what you mean by the design spanning the browsing context tree. It's not explicitly spelled out in the explainer, but the bookkeeping only happens in the top-level browsing contexts. That's because we only record it when a browsing context switch would happen, ie only when navigating a top level browsing context. We are not putting things in child browsing contexts, because I thought this would make the bookkeeping very complicated.

At the same time, to make reporting useful we have to consider the cases where same-origin iframes are initiating the problematic accesses themselves. So we should generate reports for violations coming from same-origin iframes. Since I didn't want to put additional bookkeeping in the child browsing contexts, the WindowProxy part is a bit complicated as we have to get to the top level frames to check if we need to report accesses in between the two. Not to mention that we should also exclude cross-origin iframes from the reporting mechanism.

Now, we can change this design and propagate the bookkeeping information down to child frames, and it will simplify the checks in WindowProxy. However, tracking who needs to report what to whom is going to get more complicated (as opposed to updating things every time we navigate top-level).

annevk commented 4 years ago

I think the bits I found confusing were "Other documents in the browsing context group URL for reporting" and "Report blocked accesses to other windows", but I should probably read through it again.

From your reply, I agree that we need to deal with framed first-party documents and I don't see a reason to propagate state to them as they have synchronous access to top-level state (by virtue of being first party).

camillelamy commented 4 years ago

So for the URLs, what I had in mind is that you use the appropriate URL based on your top-level document, but I see now that it is not written explicitly. In this context, "Other documents in the browsing context group URL for reporting" was meant for documents whose top-level browsing context is not in an opener/openee relationship with the COOP document.

annevk commented 4 years ago

Reading through things I still don't quite understand the need for all the bookkeeping and especially browsingContextsToNotifyOfAccess. It seems that when you have a [[Get]] or [[Set]] it's relatively straightforward to get the two environments involved. From there you can get the top-level environments that have the relevant metadata.

I understand we might need to store some additional things for popups (such as their initial URL and perhaps who has access to that), but do we need anything else?

camillelamy commented 4 years ago

Re-reading the design it might indeed be possible to simplify things. I originally wrote it for enforcement mode, which required quite a lot of bookkeeping, but if we are doing only report-only mode we do have access to the browsing contexts themselves and so we might be able to do something simpler.

I do think we need to track the initial popup URL, and this is can be placed in the top-level browsing context of the popup since it will not change. We also need to keep track of when browsing context group switches would have happened with COOP report-only (the virtual bcg id part). This should also stay on top level browsing context groups.

camillelamy commented 4 years ago

I have updated the explainer to make the design simpler.