Fix sandboxed iframes to load correct content #37593
Conversation
TimvdLippe
left a comment
TimvdLippe
left a comment
There was a problem hiding this comment.
See the comments in order to read through the code. While it avoids the CSP error, it doesn't actually load the iframe and I don't understand why. @jdm any thoughts on where the sandbox could have an effect? I feel like we are missing some sort of "hey, go start doing things now" in the iframe. Since it seems like it correctly navigates to the URL and then simply shows a blank screen and does nothing.
| self.initiate_load_based_on_url(new_load); | ||
| } | ||
|
|
||
| fn initiate_load_based_on_url(&self, load: InProgressLoad) { |
There was a problem hiding this comment.
This is taken from #36529 (comment)
While this avoids the CSP error as reported in #37499, it doesn't correctly do what the user would expect. On the MDN page, or any sandboxed iframe in WPT tests, the iframe remains empty. E.g. it is as is it never starts.
|
|
||
| let (event_loop, host) = match sandbox { | ||
| IFrameSandboxState::IFrameSandboxed => (None, None), | ||
| IFrameSandboxState::IFrameSandboxed => { |
There was a problem hiding this comment.
Then I thought: if it doesn't start, maybe the special case here is the culprit. Since this is the only place where I can see the sandbox attribute have an effect. Here, my assumption was: we don't create an event loop in sandbox iframes, so it never starts.
Unfortunately, I still see a blank screen.
|
|
||
| let scheme = url.scheme(); | ||
| match scheme { | ||
| "about" if url.path() == "srcdoc" => unreachable!(), |
There was a problem hiding this comment.
I can confirm that we load the correct URL, as with a print in htmliframeelement, the correct URL is processed. And we don't hit this panic.
|
Ok, I think the problem from #37499 on main is that when a non-sandboxed iframe has a src URL, we handle the initial about:blank entirely within the script thread without going to the network thread (so no CSP check happens against about:blank), and then the proper src URL goes to the network thread and passes the CSP check. A sandboxed iframe starts an about:blank load in a new script thread and that goes to the network thread and fails the CSP check. I'm going to try to investigate the changes in this PR later today. |
|
Weirdly, when runs from the iframe's post-connection steps, the URL we try to load is about:blank. This suggests to me that the iframe's URL is changed later, but maybe this JS exception prevents the code from running that changes the URL: |
|
I created a dummy HTML page to test this with a static |
|
🔨 Triggering try run (#15948085161) for Linux (WPT) |
|
Test results for linux-wpt from try job (#15948085161): Flaky unexpected result (14)
Stable unexpected results that are known to be intermittent (28)
Stable unexpected results (34)
|
|
|
|
The results of |
If you're referring to these failures: That comes from the document being initialized with servo/components/script/script_thread.rs Lines 3853 to 3854 in 9a0f2be servo/components/script/script_thread.rs Line 3256 in 9a0f2be |
|
Ooh, and servo/components/script/dom/document.rs Lines 814 to 829 in 9a0f2be |
There was a discrepancy that for iframes with a sandbox, the script_thread wouldn't handle the `about:` URLs correctly. Now, we reuse the same logic as used when navigating in an already present child frame. Fixes servo#36529 Fixes servo#37499 Signed-off-by: Tim van der Lippe <[email protected]>
This is used in the fallback base URL for about:blank and about:srcdoc documents. Signed-off-by: Tim van der Lippe <[email protected]>
TimvdLippe
force-pushed
the
pr-timvdlippe-handle-iframe-sandbox
branch
from
b0c3581 to
bf3ca58
Compare
|
Pushed my attempt on realigning the implementation to the spec. Unfortunately it doesn't work yet, as the URL is |
This isn't very surprising to me—this part of the specification (including the lines you quoted in the issue) is much newer than most of the navigation-related code in Servo. |
|
Okay, it makes sense to me that the code predates the spec. I am surprised as to (seemingly to me at least) the degree they diverge. This might be my lack of experience working with network code, as I don't see a lot of similarities with the code as it is and what the spec expects. Whenever I venture into the constellation or iframe network code, I feel like I hit a brick wall. Then I wonder what's causing that to happen: is it my lack of experience with network code, lack of experience with building browsers, or is this too complicated for me to pick up at this stage? With scripting code, I feel like I can manage, with the odd Rust bit that I need learn (looking at you Do let me know if I am missing something fundamental and how I can more knowledge about that. I am eager to learn, but at the same time the brick wall is still there and I don't know how to get around it. |
|
A lot of the network code was written alongside the original Fetch specification (and evolved from a non-Fetch implementation), and there have been a lot of changes to the specification since that implementation. Likewise, the constellation is one of the earliest pieces of Servo (I remember describing it in technical talks in 2013), and encompasses some complex parts of the specification like session history that have also undergone extensive revisions in the past five years, while the constellation implementation has largely remained unchanged. My experience is that navigation, session history, and async fetching are already very complex parts of the specification to reason about, and what exists in the specification has long evolved in a different direction than Servo's implementation (despite our influence like whatwg/html#1454 (comment)). This makes it particularly unapproachable if you're used to using the specification text to help guide your understanding of Servo's code, which is a much easier experience in must the script crate (as you have found!). #33087 is an example of my experiments to bring more of our iframe loading implementation in line with the specification. It's a lot of trial and error to figure out where are the right places to make changes, unfortunately. I don't have good suggestions right now, but I may try writing down some high level documentation about what Servo does today so it's easier to see the big picture. |
|
In particular, whatwg/html#6315 was a huge change to the specification around navigation and session history and occurred while Servo was mostly dormant. As a result we haven't dedicated much effort to adjusting our implementation to catch up with it, leading to the current difficulties of determining how to make correctness changes to Servo's implementation. |
|
Closing this in favor of #39610 CC @mrobinson |
|
@TimvdLippe Oh, I didn't realize these two changes were conflicting. Apologies for stepping on your toes here. |
|
You weren't. I had sort of given up on this PR already as I realized the iframe machinery needs some love first. So I am actually glad you took over! |
3 participants
There was a discrepancy that for iframes with a sandbox, the script_thread wouldn't handle the
about:URLs correctly. Now, we reuse the same logic as used when navigating in an already present child frame.Fixes #36529
Fixes #37499