Bug 41165

Created attachment 65432 [details] patch This change is large, but all mechanical except for FrameLoader and Document::Document(). There are a couple of places in FrameLoader where we're logging the url, but the document might be detached, so there are some null checks that need to happen that weren't necessary previously. Also, ordering of setting variables needed to change in a couple of places. The changes in Document's constructor are tweaking the handling of about:blank. I went for full backwards compatibility as defined by the existing layout tests.

Comment on attachment 65432 [details] patch Great idea to do this! Unlike loader(), there is a tiny window where a frame can have 0 for document(). I’m hoping we won’t introduce any new null-dereference problems with this change. It seems to me that could have been done in two steps. One step would be about removing FrameLoader::m_URL, and a separate step would be removing FrameLoader::url. This would make one patch have all the substantive changes, and another just the mechanical step of calling directly through document() instead of through loader(), making it easier to review. > + // We need to be careful about initializing the url in the case where we're passed an empty url. > + // Specifically, we don't want to set about:blank for the initial empty document unless it's not the main frame. > + if (!url.isEmpty() || (frame && (!frame->loader()->stateMachine()->creatingInitialEmptyDocument() || frame->tree()->parent()))) > setURL(url); This comment should be replaced by one that says why we don't want to set about:blank for the initial empty document on the main frame. Comments should focus on why the code does something, not what the code does. The code itself should be written so that it's clear what it does. he code already says what the comment says -- don't set the URL to the empty URL if it's an initial empty document for the main frame -- so we don't need to repeat that. What we need from the comment is the information *not* contained in the code. I do not like the phrase "be careful" here, or frankly ever in discussions of computer software. We should always be careful! ;-) The word is "URL" not "url" in plain language, despite the fact that we use "url" in variable names. Otherwise, the patch seems good. r=me, but I’m not going to set commit-queue+ because I’d like to see the comment replaced by a substantive one.

(In reply to comment #2) > (From update of attachment 65432 [details]) > Great idea to do this! > > Unlike loader(), there is a tiny window where a frame can have 0 for document(). I’m hoping we won’t introduce any new null-dereference problems with this change. I wasn't able to find any cases of null documents other than the ones I mentioned in passing in comment #1. > > It seems to me that could have been done in two steps. One step would be about removing FrameLoader::m_URL, and a separate step would be removing FrameLoader::url. This would make one patch have all the substantive changes, and another just the mechanical step of calling directly through document() instead of through loader(), making it easier to review. I think I may end up doing that after all (see below) > > > + // We need to be careful about initializing the url in the case where we're passed an empty url. > > + // Specifically, we don't want to set about:blank for the initial empty document unless it's not the main frame. > > + if (!url.isEmpty() || (frame && (!frame->loader()->stateMachine()->creatingInitialEmptyDocument() || frame->tree()->parent()))) > > setURL(url); > > This comment should be replaced by one that says why we don't want to set about:blank for the initial empty document on the main frame. > > Comments should focus on why the code does something, not what the code does. The code itself should be written so that it's clear what it does. he code already says what the comment says -- don't set the URL to the empty URL if it's an initial empty document for the main frame -- so we don't need to repeat that. What we need from the comment is the information *not* contained in the code. So part of the reason the comment was light on details on the 'why' of the change was that there is some voodoo to this code that I don't fully understand. I think part of it is that I can't decide whether the initial empty document created in FrameLoader::init() should have an empty or about:blank url. Either way, I have one failing test that needs to be resolved. If it should be empty, then we either need both of the checks I added in Document's constructor or we need the isCreatingInitialDocument() check and fast/dom/early-frame-url.html needs to be rewritten (I think the test is not right in its current state, since it appears to be busy-waiting expecting the url to change and not letting the stack unwind so the navigation can occur). If it should be about:blank, then we end up hitting an assert (http://trac.webkit.org/browser/trunk/WebCore/loader/HistoryController.cpp#L102) in http://trac.webkit.org/browser/trunk/LayoutTests/fast/dom/location-new-window-no-crash.html, apparently because it is dependent on the url of a created window starting as empty (if it's about:blank, it does some navigations synchronously because they're the same document and the test has already outputted some FAIL messages before the assert is hit). I'm inclined to say the right answer is that the initial empty document should have an empty url and early-frame-url.html needs rewriting, but I'm quite willing to be persuaded otherwise.

Created attachment 65597 [details] split patch - remove m_URL and have FrameLoader::url() call Document::url() This splits the patch into 2 as Darin suggested. The main reason I'm doing this is to check my thinking on the rules of initializing the url in the Document constructor and that my layout test rewriting seems sane (see my previous comment for my pontification on the topic).

Comment on attachment 65597 [details] split patch - remove m_URL and have FrameLoader::url() call Document::url() View in context: https://bugs.webkit.org/attachment.cgi?id=65597&action=prettypatch > WebCore/loader/FrameLoader.cpp:519 > + m_workingURL = url; > + if (m_workingURL.protocolInHTTPFamily() && !m_workingURL.host().isEmpty() && m_workingURL.path().isEmpty()) > + m_workingURL.setPath("/"); It seems like m_URL no longer has this modification applied. Does this code actually do anything? I would have expected KURL to normalize empty paths to include "/"... > WebCore/loader/FrameLoader.cpp:724 > if (!iconDatabase()->iconDataKnownForIconURL(urlString)) { > LOG(IconDatabase, "Told not to load icon %s but icon data is not yet available - registering for notification and requesting load from disk", urlString.ascii().data()); > m_client->registerForIconNotification(); > - iconDatabase()->iconForPageURL(m_URL.string(), IntSize(0, 0)); > + iconDatabase()->iconForPageURL(m_frame->document()->url().string(), IntSize(0, 0)); why not url() in all these places? > WebCore/loader/FrameLoader.cpp:1829 > - LOG(PageCache, "WebCoreLoading %s: About to commit provisional load from previous URL '%s' to new URL '%s'", m_frame->tree()->name().string().utf8().data(), m_URL.string().utf8().data(), > + LOG(PageCache, "WebCoreLoading %s: About to commit provisional load from previous URL '%s' to new URL '%s'", m_frame->tree()->name().string().utf8().data(), > + m_frame->document() ? m_frame->document()->url().string().utf8().data() : "", I presume you checked that document can actually be null here. > WebCore/loader/FrameLoader.cpp:1870 > - LOG(Loading, "WebCoreLoading %s: Finished committing provisional load to URL %s", m_frame->tree()->name().string().utf8().data(), m_URL.string().utf8().data()); > + LOG(Loading, "WebCoreLoading %s: Finished committing provisional load to URL %s", m_frame->tree()->name().string().utf8().data(), > + m_frame->document() ? m_frame->document()->url().string().utf8().data() : ""); and here. > WebCore/loader/FrameLoader.cpp:2113 > m_workingURL = url; By the way, m_workingURL seems wrong too. Shouldn't that state be held in the DocumentLoader? > LayoutTests/fast/dom/resources/a.html:9 > + else if (window.layoutTestController) > + layoutTestController.notifyDone(); Indent issue? > LayoutTests/ChangeLog:6 > + Update a test that busy-waits when it shouldn't. > + https://bugs.webkit.org/show_bug.cgi?id=41165. This doesn't appear to belong to this patch.

(In reply to comment #5) > (From update of attachment 65597 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=65597&action=prettypatch > > > WebCore/loader/FrameLoader.cpp:519 > > + m_workingURL = url; > > + if (m_workingURL.protocolInHTTPFamily() && !m_workingURL.host().isEmpty() && m_workingURL.path().isEmpty()) > > + m_workingURL.setPath("/"); > It seems like m_URL no longer has this modification applied. Does this code actually do anything? I would have expected KURL to normalize empty paths to include "/"... No idea, I'll check. > > > WebCore/loader/FrameLoader.cpp:724 > > if (!iconDatabase()->iconDataKnownForIconURL(urlString)) { > > LOG(IconDatabase, "Told not to load icon %s but icon data is not yet available - registering for notification and requesting load from disk", urlString.ascii().data()); > > m_client->registerForIconNotification(); > > - iconDatabase()->iconForPageURL(m_URL.string(), IntSize(0, 0)); > > + iconDatabase()->iconForPageURL(m_frame->document()->url().string(), IntSize(0, 0)); > why not url() in all these places? No good reason, just leftover from the previous version. > > > WebCore/loader/FrameLoader.cpp:1829 > > - LOG(PageCache, "WebCoreLoading %s: About to commit provisional load from previous URL '%s' to new URL '%s'", m_frame->tree()->name().string().utf8().data(), m_URL.string().utf8().data(), > > + LOG(PageCache, "WebCoreLoading %s: About to commit provisional load from previous URL '%s' to new URL '%s'", m_frame->tree()->name().string().utf8().data(), > > + m_frame->document() ? m_frame->document()->url().string().utf8().data() : "", > I presume you checked that document can actually be null here. > > > WebCore/loader/FrameLoader.cpp:1870 > > - LOG(Loading, "WebCoreLoading %s: Finished committing provisional load to URL %s", m_frame->tree()->name().string().utf8().data(), m_URL.string().utf8().data()); > > + LOG(Loading, "WebCoreLoading %s: Finished committing provisional load to URL %s", m_frame->tree()->name().string().utf8().data(), > > + m_frame->document() ? m_frame->document()->url().string().utf8().data() : ""); > and here. Yes to both. > > > WebCore/loader/FrameLoader.cpp:2113 > > m_workingURL = url; > By the way, m_workingURL seems wrong too. Shouldn't that state be held in the DocumentLoader? Agreed. I fooled around with it briefly a couple days ago, but it wasn't trivial. I'm planning on playing with it more at a later date. > > > LayoutTests/fast/dom/resources/a.html:9 > > + else if (window.layoutTestController) > > + layoutTestController.notifyDone(); > Indent issue? > > > LayoutTests/ChangeLog:6 > > + Update a test that busy-waits when it shouldn't. > > + https://bugs.webkit.org/show_bug.cgi?id=41165. > This doesn't appear to belong to this patch. That was intentional, but probably a poorly written ChangeLog :) That test makes ordering assumptions that appear to be no longer valid. See Comment #3.

http://trac.webkit.org/changeset/66671 might have broken Qt Linux Release

> I think the test is not right in its current state Did the test use to pass in other browsers?

(In reply to comment #8) > > I think the test is not right in its current state > > Did the test use to pass in other browsers? I'll double-check, the patch is currently in the rollout queue because it broke a different test on a bunch of platforms.

Comment on attachment 65432 [details] patch Cleared Darin Adler's review+ from obsolete attachment 65432 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Hi Nate, I have a bug (41155) that is hold until this patch is landed to determine whether the issue reported in 41155 will be fixed by your changes. Do you know how long it will take to land your patch? Thanks, Dinu

(In reply to comment #11) > Hi Nate, > > I have a bug (41155) that is hold until this patch is landed to determine whether the issue reported in 41155 will be fixed by your changes. Do you know how long it will take to land your patch? > Thanks, Dinu It may be a while. There's a corner case that breaks with the version I committed and reverted, and I haven't figured out how to resolve it. I got frustrated and decided to work on something else for a while, so it's fairly unlikely I'll resolve this in the next 2 weeks.

(In reply to comment #12) > (In reply to comment #11) > > Hi Nate, > > > > I have a bug (41155) that is hold until this patch is landed to determine whether the issue reported in 41155 will be fixed by your changes. Do you know how long it will take to land your patch? > > Thanks, Dinu > > It may be a while. There's a corner case that breaks with the version I committed and reverted, and I haven't figured out how to resolve it. I got frustrated and decided to work on something else for a while, so it's fairly unlikely I'll resolve this in the next 2 weeks. Thanks Nate.

3-month ping?

(In reply to comment #14) > 3-month ping? I looked at this again a couple of weeks ago and wasn't able to get it working. There's still a corner case where we're failing. :(

Created attachment 80201 [details] patch without any test changes The only substantive code change from the previous patch is the check of frame->ownerElement() in Document::Document() and the comment explain it. This patch passes all existing layout tests as-is, unlike previous iterations.

Comment on attachment 80201 [details] patch without any test changes View in context: https://bugs.webkit.org/attachment.cgi?id=80201&action=review Yay! Thanks for coming back to this patch. > Source/WebCore/dom/Document.cpp:453 > - if (frame || !url.isEmpty()) > + // We depend on the url getting immediately set in subframes, but we > + // also depend on the url NOT getting immediately set in opened windows. > + // See fast/dom/early-frame-url.html > + // and fast/dom/location-new-window-no-crash.html, respectively. > + // FIXME: Can/should we unify this behavior? > + if ((frame && frame->ownerElement()) || !url.isEmpty()) > setURL(url); Crazy! > Source/WebCore/loader/FrameLoader.cpp:-541 > - if (m_frame->document()->url() != blankURL()) > - m_URL = m_frame->document()->url(); Woah. This old code was wrong! > Source/WebCore/loader/FrameLoader.cpp:650 > KURL ref(url); > ref.setUser(String()); > ref.setPass(String()); > ref.removeFragmentIdentifier(); > m_outgoingReferrer = ref.string(); ref is kind of a silly name for this variable. Maybe "outgoingReferrer" ?

(In reply to comment #17) > (From update of attachment 80201 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80201&action=review > > Yay! Thanks for coming back to this patch. > > > Source/WebCore/dom/Document.cpp:453 > > - if (frame || !url.isEmpty()) > > + // We depend on the url getting immediately set in subframes, but we > > + // also depend on the url NOT getting immediately set in opened windows. > > + // See fast/dom/early-frame-url.html > > + // and fast/dom/location-new-window-no-crash.html, respectively. > > + // FIXME: Can/should we unify this behavior? > > + if ((frame && frame->ownerElement()) || !url.isEmpty()) > > setURL(url); > > Crazy! > > > Source/WebCore/loader/FrameLoader.cpp:-541 > > - if (m_frame->document()->url() != blankURL()) > > - m_URL = m_frame->document()->url(); > > Woah. This old code was wrong! > > > Source/WebCore/loader/FrameLoader.cpp:650 > > KURL ref(url); > > ref.setUser(String()); > > ref.setPass(String()); > > ref.removeFragmentIdentifier(); > > m_outgoingReferrer = ref.string(); > > ref is kind of a silly name for this variable. Maybe "outgoingReferrer" ? outgoingReferrer sounds good. Will fix up on landing.

Comment on attachment 80201 [details] patch without any test changes Landed: http://trac.webkit.org/changeset/76702 Leaving this bug open, since we still need to get rid of FrameLoader::url(). That patch is next on my list.

http://trac.webkit.org/changeset/76702 might have broken Leopard Intel Release (Tests)

Created attachment 80231 [details] Remove FrameLoader:url()

Comment on attachment 80231 [details] Remove FrameLoader:url() Rejecting attachment 80231 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'bu..." exit_code: 2 Last 500 characters of output: e/WebCore.build/Objects-normal/x86_64/DOMHTMLStyleElement.o /Projects/CommitQueue/WebKitBuild/Release/DerivedSources/WebCore/DOMHTMLStyleElement.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2 CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/DOMHTMLTableCaptionElement.o /Projects/CommitQueue/WebKitBuild/Release/DerivedSources/WebCore/DOMHTMLTableCaptionElement.mm normal x86_64 objective-c++ com.apple.compilers.gcc.4_2 (2 failures) Full output: http://queues.webkit.org/results/7548370

Attachment 80231 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7509347

Attachment 80231 [details] did not build on qt: Build output: http://queues.webkit.org/results/7614356

Created attachment 80245 [details] Fix qt and gtk issues

The commit-queue encountered the following flaky tests while processing attachment 80245 [details]: http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 80245 [details] Fix qt and gtk issues Clearing flags on attachment: 80245 Committed r76872: <http://trac.webkit.org/changeset/76872>

All reviewed patches have been landed. Closing bug.