Issue metadata
Sign in to add a comment
|
Frame::SetIsLoading(true) is called at the wrong time. |
||||||||||||||||||||
Issue descriptionI think that I observe the following problem: 1. Visit a website that contains a mainframe in one renderer [1] and an iframe that is hosted in a separate renderer process [2]. 2. Post a form in the mainframe that triggers a navigation to the same site. Observe that - blink::Frame::SetIsLoading(true) is called for the mainframe (WebRemoteFrame) in [2] - blink::Frame in [2] is replaced with a new instance for which blink::Frame::is_loading_ set to false - this is not expected - when the navigation finishes, blink::Frame::SetIsLoading(false) is called for the frame that already has is_loading_ set to false. Here are the reproduction steps: - Visit https://www.topcashback.co.uk/ - Type "user" and "password" into the signup form - Press "Join Now" This is the analysis: https://pastebin.com/KM7bD509 The site structure is the following: - main_frame - iframe pointing to youtube.com - iframe pointing to about:blank The log starts when I press the "Join Now" button. The log contains two columns. - On the left you see log output from the renderer process that hosts the main frame [1]. - On the right you see log output from the renderer process that hosts the youtube frame [2]. At line 4, you see that Frame::SetIsLoading(true) is called as expected for the (local) main frame in [1]. The following lines 5-25 show the frame tree with is_loading state *before* the update. At line 34, you see that Frame::SetIsLoading(true) is also called for the (remote) main frame in [2]. Now the inner frames are replaced with new content. At line 70 you see how the about:blank frame is set to is_loading=true and a line 67 you see that is_loading was updated to true for the main_frame in [1]. Now the second renderer [2] observes the SetIsLoading(true) for the about:blank frame (line 84). But here is the bug: The (remote) main frame does not have is_loading=1. This is because the actual instance of the blink::Frame object has been replaced. It was located at 0xaf3a4dc1f18 (see line 36) and replaced with a new blink::Frame object at 0xaf3a4e77698 (see line 86). That one missed the is_loading update. Just for funsies: In line 669, the is_loading state is set from 0 to 0. I.e. the update saying that the main frame stopped loading happens in [2] but the is_loading state was never set to true. Line 705 and following show the stack trace of where the RemoteFrame representing the main frame in [2] is replaced. Dmitry / Daniel, could you please take a look? This blocks one of my bugs. Thanks a lot.
,
Dec 18
One more addendum: At first I thought that this could be fixed by cloning the is_loading_ flag in WebFrame::Swap but I guess it is more complicated because the entire renderer process can be replaced.
,
Dec 18
Adding frame->frame_->SetIsLoading(true); to WebRemoteFrameImpl::CreateMainFrame before the return statement solves my problems for 914327 but I am not sure whether it would be legitimate to assume that a new remote mainframe is always loading in the beginning.
,
Dec 18
Great analysis, thanks for filing! Perhaps, we are not plumbing |is_loading| bit from FrameTreeNode::IsLoading() when creating a new RenderFrameProxy for the remote main frame, but we should? Over to Daniel to decide, also +creis FYI.
,
Jan 2
Happy new year! Pinging this bug to bring it back in your inbox.
,
Jan 9
The NextAction date has arrived: 2019-01-09
,
Jan 11
Friendly ping at dcheng@
,
Jan 11
,
Jan 11
,
Jan 16
(6 days ago)
The NextAction date has arrived: 2019-01-16
,
Jan 17
(5 days ago)
Friendly ping at dcheng@
,
Jan 17
(5 days ago)
Alex, maybe you can take a look? It seems like this repro case is partly similar to issue 894253, in terms of doing a same-site navigation with the same OOPIF being created both times. IIUC, we're going from A1(B1) to A2(B2). (Technically, the destination is actually a repeat visit to A1(B1) again, but the problem seems general to any same-site pages in the main frame and subframe.) When we navigate cross-document, we tear down the B1 OOPIF and all proxies in B's process, because the new A2 document hasn't made any subframes yet. However, I think the browser process keeps around B's process for a short time to let it finish up any unload handlers, etc. Thus, when the new A2 page later creates its B2 subframe, the B SiteInstance ends up using the existing B process for B2 (and the new proxy for the main frame). In that sense, I'm not surprised we create a new proxy in this case. In fact, if A2 took a few seconds before creating its B2 subframe, the B process might have gone away and we'd need to create a new process from scratch for it. However, I agree with battre@-- it seems surprising that the main frame proxy in B's process doesn't know that the main frame is in a loading state. Shouldn't we be replicating the loading state in this case, given that the FrameTreeNode for A2 (presumably) knows it is loading? Maybe that proxy should be created in a state where is_loading is true, avoiding a separate SetIsLoading call for it? |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by battre@chromium.org
, Dec 18