Bug 41165 – FrameLoader cleanup : Remove m_URL member

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

FrameLoader::url() should always be the same as Document::url(), so it's redundant.  There appears to be one corner case in initialization of Documents where this isn't the case, but once that is resolved this variable should be able to be deleted.

[Nate Chapin]

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.

[Darin Adler]

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.

[Nate Chapin]

(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.

[Nate Chapin]

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).

[Adam Barth]

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.

[Nate Chapin]

(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.

[WebKit Review Bot]

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

[Alexey Proskuryakov]

> I think the test is not right in its current state

Did the test use to pass in other browsers?

[Nate Chapin]

(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.

[Eric Seidel (no email)]

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.

[Dinu Jacob]

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

[Nate Chapin]

(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.

[Dinu Jacob]

(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.

[Eric Seidel (no email)]

3-month ping?

[Nate Chapin]

(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. :(

[Nate Chapin]

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.

[Adam Barth]

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" ?

[Nate Chapin]

(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.

[Nate Chapin]

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.

[WebKit Review Bot]

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

[Nate Chapin]

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

[WebKit Commit Bot]

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

[WebKit Review Bot]

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

[Early Warning System Bot]

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

[Nate Chapin]

Created attachment 80245 [details]
Fix qt and gtk issues

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 80245 [details]:

http/tests/xmlhttprequest/cross-origin-authorization.html bug 52398 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 80245 [details]
Fix qt and gtk issues

Clearing flags on attachment: 80245

Committed r76872: <http://trac.webkit.org/changeset/76872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.