Issue metadata
Sign in to add a comment
|
We pass URLs from the renderer to the browser even though the browser already knows them |
||||||||||||||||||||||||
Issue description(Repurposing this bug. Originally it was about a DCHECK in dialogs, but it's fundamentally an issue with navigation, and I'm removing the DCHECK.) When a document uses `document.write` on an about:blank page, it's supposed to alter the URL of the page written to. This correctly happens on the renderer side, but the browser side is never updated to match. It should be.
,
May 5 2017
There are two URLs in the DCHECK. One is RenderFrameHostImpl's last_committed_url_, the other is passed in from the render side, the blink::WebLocalFrame's Document's URL. RenderFrameHostImpl's last_committed_url_ is about:blank because the FrameTreeNode's current_url() is about:blank, because Navigator::DidNavigate was called with about:blank, because RenderFrameHostImpl::OnDidCommitProvisionalLoad was called with the url parameter about:blank. Nothing changed the URL of that frame on the browser side after that initial commit. I'm working on seeing how the blink::WebLocalFrame's Document's URL ended up being something that wasn't about:blank.
,
May 5 2017
Cool. I'm guessing this is the second of the two subframes on the page? DevTools says its location.href is "https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_onbeforeunload". Looks like document.write is to blame. :( That appears to change the location.href for the document in the renderer process, without generating a commit message to the browser. That's probably how the two URLs diverge. I wonder if this is going to cause us other pain with last_committed_url_... I know Nasko and I have hit problems there before.
,
May 5 2017
That sounds like it's it. You know about this one?
,
May 5 2017
Do we have tests covering the failure of document.write to update the url on the browser side?
,
May 5 2017
BTW, confirmed. The code that sets up the frame on the right of the page is submitTryIt, which is...
function submitTryit(n) {
// ...
var ifrw = (ifr.contentWindow) ? ifr.contentWindow : (ifr.contentDocument.document) ? ifr.contentDocument.document : ifr.contentDocument;
ifrw.document.open();
ifrw.document.write(text);
ifrw.document.close();
// ...
}
}
,
May 5 2017
,
May 8 2017
Unassigning this from myself, as I will not be able to look at it in the next 2-3 days.
,
May 25 2017
,
May 25 2017
BTW, we've indirectly hit the problem of document.write altering the renderer-side URL in https://crbug.com/527367#c11 and verified that this is spec-compliant. There's a test that davidsac@ added which exercises it, though it doesn't cover the lack of update to the browser side.
,
Jun 21 2017
One hint: in recent debugging sessions, I've noticed that OnRunBeforeUnloadConfirm always seems to arrive twice for each navigation.
,
Jun 21 2017
nick@ is the double messaging happening with/without PlzNavigate, or both?
,
Jun 21 2017
Seems to be both with and without based on local testing. It's probably a separate bug from this one, which seems to be well-understood. I'll file another issue. For this bug, is the preferred approach here to update the URL in the browser process FrameTreeNode, when the document.write operation occurs? If so, could someone sketch out what that would look like?
,
Jun 21 2017
I was just chatting with some of the networking folks about the document.write case when it comes to navigations. Some ideas came up and I'm planning to write it up in a doc for people to comment on. It might touch on this area as well, so I think it makes sense to discuss it all in the same doc. I will post a link once I have the basics written up.
,
Aug 27 2017
,
Mar 8 2018
FYI, I added a similar check to the JS dialog handler, and it's DCHECKing in a similar manner.
,
Mar 8 2018
This is also happening on Android. Also noticed that since it uses last_committed_url_ which is different from frame_url in this case, the title of the dialog is not correct. Some logs that might be helpful: 03-08 02:21:13.627 13534-13534/org.chromium.chrome E/chromium: [ERROR:render_frame_host_impl.cc(1433)] 0xd0e6c600, SetLastCommittedUrl: https://www.w3schools.com/js/tryit.asp?filename=tryjs_confirm 03-08 02:21:13.903 13534-13534/org.chromium.chrome E/chromium: [ERROR:render_frame_host_impl.cc(1433)] 0xd1228a00, SetLastCommittedUrl: about:blank 03-08 02:21:14.585 13534-13534/org.chromium.chrome E/chromium: [ERROR:render_frame_host_impl.cc(1433)] 0xd122ad00, SetLastCommittedUrl: about:srcdoc 03-08 02:21:14.724 13534-13534/org.chromium.chrome E/chromium: [ERROR:render_frame_host_impl.cc(1433)] 0xd1264200, SetLastCommittedUrl: about:blank 03-08 02:21:15.002 13534-13534/org.chromium.chrome E/chromium: [ERROR:render_frame_host_impl.cc(2073)] 0xd1228a00, OnRunJavaScriptDialog 03-08 02:21:15.040 13534-13534/org.chromium.chrome A/chromium: [FATAL:render_frame_host_impl.cc(2074)] Check failed: frame_url == last_committed_url_ (https://www.w3schools.com/js/tryit.asp?filename=tryjs_confirm vs. about:blank) #00 0xd4646785 /data/data/org.chromium.chrome/incremental-install-files/lib/libbase.cr.so+0x000cd785 #01 0xcb780aa5 /data/data/org.chromium.chrome/incremental-install-files/lib/libcontent.cr.so+0x00be8aa5 #02 0xcb78b48b /data/data/org.chromium.chrome/incremental-install-files/lib/libcontent.cr.so+0x00bf348b #03 0xcb78089b /data/data/org.chromium.chrome/incremental-install-files/lib/libcontent.cr.so+0x00be889b #04 0xcb77f18b /data/data/org.chromium.chrome/incremental-install-files/lib/libcontent.cr.so+0x00be718b #05 0xd292dd29 /data/data/org.chromium.chrome/incremental-install-files/lib/libipc.cr.so+0x00016d29 #06 0xd462d735 /data/data/org.chromium.chrome/incremental-install-files/lib/libbase.cr.so+0x000b4735 #07 0xd4635f4b /data/data/org.chromium.chrome/incremental-install-files/lib/libbase.cr.so+0x000bcf4b #08 0xd464a377 /data/data/org.chromium.chrome/incremental-install-files/lib/libbase.cr.so+0x000d1377 #09 0xd464bec1 /data/data/org.chromium.chrome/incremental-install-files/lib/libbase.cr.so+0x000d2ec1 #10 0xd464c0cf /data/data/org.chromium.chrome/incremental-install-files/lib/libbase.cr.so+0x000d30cf #11 0xd464c19d /data/data/org.chromium.chrome/incremental-install-files/lib/libbase.cr.so+0x000d319d #12 0xd464c94f /data/data/org.chromium.chrome/incremental-install-files/lib/libbase.cr.so+0x000d394f #13 0xd464c923 /data/data/org.chromium.chrome/incremental-install-files/lib/libbase.cr.so+0x000d3923 03-08 02:21:15.041 13534-13534/org.chromium.chrome A/libc: Fatal signal 6 (SIGABRT), code -6 in tid 13534 (chromium.chrome) [ 03-08 02:21:15.041 166: 166 W/ ] debuggerd: handling request: pid=13534 uid=10053 gid=10053 tid=13534
,
Mar 8 2018
,
Mar 8 2018
,
Mar 8 2018
,
Mar 8 2018
Actually, since there already is a bug for this, bug 759356, let's use that, and reuse this one for pulling out the plumbing we don't need.
,
Mar 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2060a042c8e7746fea6ec0ae19ffe736e258c208 commit 2060a042c8e7746fea6ec0ae19ffe736e258c208 Author: Avi Drissman <avi@chromium.org> Date: Fri Mar 09 02:02:47 2018 Drop alert DCHECKs. These DCHECKs don’t tell us anything other than that we fail to update the browser-side notion of location when a document writes into an about:blank document, but we know that. BUG= 820063 , 718570 Change-Id: I469d75afaafcd87c98529037a1fbbca155ce806c Reviewed-on: https://chromium-review.googlesource.com/956282 Reviewed-by: Nasko Oskov <nasko@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#541989} [modify] https://crrev.com/2060a042c8e7746fea6ec0ae19ffe736e258c208/content/browser/frame_host/render_frame_host_impl.cc
,
Mar 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/21c0090691e9a7e92ab6f513f4ea312785ab643d commit 21c0090691e9a7e92ab6f513f4ea312785ab643d Author: Avi Drissman <avi@chromium.org> Date: Fri Mar 09 18:28:31 2018 Stop sending frame URLs to the browser process. It knows them already, and the case where it doesn’t doesn’t matter for this and we already plan on fixing it. BUG= 718570 Change-Id: Ic9cc32a265cff867459ce031699c453fcbe7baf0 Reviewed-on: https://chromium-review.googlesource.com/956283 Reviewed-by: Ken Buchanan <kenrb@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#542167} [modify] https://crrev.com/21c0090691e9a7e92ab6f513f4ea312785ab643d/content/browser/frame_host/render_frame_host_impl.cc [modify] https://crrev.com/21c0090691e9a7e92ab6f513f4ea312785ab643d/content/browser/frame_host/render_frame_host_impl.h [modify] https://crrev.com/21c0090691e9a7e92ab6f513f4ea312785ab643d/content/common/frame_messages.h [modify] https://crrev.com/21c0090691e9a7e92ab6f513f4ea312785ab643d/content/renderer/render_frame_impl.cc [modify] https://crrev.com/21c0090691e9a7e92ab6f513f4ea312785ab643d/content/renderer/render_frame_impl.h
,
Mar 9 2018
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by creis@chromium.org
, May 4 2017Components: UI>Browser>Navigation