Bug 27444 – Wrong FrameLoader::activeDocumentLoader when loading an invalid URL.

• Component: Page Loading
• Status: RESOLVED INVALID

[Antonio Gomes]

Created attachment 33081 [details]
set the provisional documentLoader as the activeDocument in case of load failures (v0.1)

Steps to reproduce:

1) load a valid url (e.g. http://google.com). Calling:

FrameLoader::activeDocumentLoader::originalURL() -> http://google.com
FrameLoader::activeDocumentLoader::url() -> http://www.google.com (resolved/redirected)

--> FROM GDB we see that the proper DocumentLoader is set to the FrameLoader after the load succeeds (see below):

gdb_$ b FrameLoader.cpp:setDocumentLoader
gdb_$ bt
FrameLoader::finishedLoading:3092
DocumentLoader::finishedLoading:353
main frame - willCloseFrame
FrameLoader::stopLoading:566
FrameLoader::transitionToCommitted:2869
*******FrameLoader::setDocumentLoader:2691 'loader=0x886bdb0'

2) after that, load an invalid url (e.g. http://abcdef.abcdef). 

--> Since it fails to load, FrameLoader::setDocumentLoader is *never* called, and so calling Frameloader::activeDocumentLoader() returns a reference to "0x886bdb0" (documentLoader of the previous loaded URL - see above). It results in wrong originalURL values:

FrameLoader::activeDocumentLoader::originalURL() -> http://google.com      <---- WRONG
FrameLoader::activeDocumentLoader::url() -> http://abcdef.abcdef

[Antonio Gomes]

btw, no side effects on the layout tests (tested w/ qt webkit on linux 32bits)

actually w/ patch in, one test stopped to timeout for me, but it is probably not related.

[Brady Eidson]

Comment on attachment 33081 [details]
set the provisional documentLoader as the activeDocument in case of load failures (v0.1)

Thanks for your interest and thanks for you patch, but I think you're chasing down a non-issue.

Most importantly, a Frame shouldn't ever consider a failed, non-committed load as it's current DocumentLoader.  Making this change would be a change in behavior that many WebKit applications do not expect. Most notably, Safari would break with this change.

Can you explain an example of an application you're working on where this change is desirable?  And why?

Also, if a layout test times out with your patch in but runs correctly without your patch, that *is* indicative of a regression with your patch.  I bet the test is catching exactly the WebKit API breakage that I alluded to.

[Antonio Gomes]

> Thanks for your interest and thanks for you patch, but I think you're chasing
> down a non-issue.

thanks for the comments.

> Most importantly, a Frame shouldn't ever consider a failed, non-committed load
> as it's current DocumentLoader.  Making this change would be a change in
> behavior that many WebKit applications do not expect. Most notably, Safari
> would break with this change.

is there any regression test for it ?

> Can you explain an example of an application you're working on where this
> change is desirable?  And why?

I should had mentioned it before, yes. I am working on https://bugs.webkit.org/show_bug.cgi?id=25867 (see patch 0.4) , which provides the url originally loaded by the frame.

in case of load failures, frameLoader->activeDocumentLoader points to a documentLoader instance of a (successful) previous load (see bug description). In this case, if i 

1) load google.com
2) load a known unexistent url
3) call originalUrl()
4) it calls FrameLoader::originalRequest() , which calls activeDocumentLoader()->originalRequest.url() => http://google.com

since activeDocumentLoader is not referring to the latest load but to the latest successful one.

so how do access the documentLoader that actually refers to the latest load (being in successful or not) ?

> Also, if a layout test times out with your patch in but runs correctly without
> your patch, that *is* indicative of a regression with your patch.  I bet the
> test is catching exactly the WebKit API breakage that I alluded to.

i mis-explained maybe, but there is one less failing test w/ the patch.

[Brady Eidson]

(In reply to comment #3)
> > Also, if a layout test times out with your patch in but runs correctly without
> > your patch, that *is* indicative of a regression with your patch.  I bet the
> > test is catching exactly the WebKit API breakage that I alluded to.
> 
> i mis-explained maybe, but there is one less failing test w/ the patch.

Ah, yes I understood you to mean that your patch introduced a failure.  Since the Mac bots are all running green right now, I assume you mean there's a Qt failure that this cleans up.  But have you tried running the Mac tests with your patch in?

> > Most importantly, a Frame shouldn't ever consider a failed, non-committed load
> > as it's current DocumentLoader.  Making this change would be a change in
> > behavior that many WebKit applications do not expect. Most notably, Safari
> > would break with this change.
> 
> is there any regression test for it ?

I don't know - Do you have a Mac machine to try with your patch? 

It's quite possible there isn't, as one thing I know this patch breaks - which is the WebKit API [WebDataSource unreachableURL] - has historically been difficult to test automatically.

If your patch *doesn't* break any Mac layout tests, then we need to take the time to add one.

> > Can you explain an example of an application you're working on where this
> > change is desirable?  And why?
> 
> I should had mentioned it before, yes. I am working on
> https://bugs.webkit.org/show_bug.cgi?id=25867 (see patch 0.4) , which provides
> the url originally loaded by the frame.
> 
> in case of load failures, frameLoader->activeDocumentLoader points to a
> documentLoader instance of a (successful) previous load (see bug description).
> In this case, if i 
> 
> 1) load google.com
> 2) load a known unexistent url
> 3) call originalUrl()
> 4) it calls FrameLoader::originalRequest() , which calls
> activeDocumentLoader()->originalRequest.url() => http://google.com
> 
> since activeDocumentLoader is not referring to the latest load but to the
> latest successful one.
> 
> so how do access the documentLoader that actually refers to the latest load
> (being in successful or not) ?

I followed your description here and looked at the other bug.  Amusingly, the main thing this patch would break is what happens in the error case, which covered by DocumentLoader::unreachableURL().

When you say "latest load", you are overloading some highly overloaded terminology.  In FrameLoader/DocumentLoader-land, the "current load" is the most recent main resource load to have been committed.  When a load fails because the url wasn't even valid, it was never committed, so the current activeDocumentLoader() remains unchanged.

Alas, information about this attempted-load-not-even-getting-started is tacked on the DocumentLoader.  Based on reading the other bug, you are happy with url() and originalURL(), but just want to know about failure cases.  What happens if you try using activeDocumentLoader()->mainDocumentError()?

[Brady Eidson]

Rereading even closer, it seems like you *WANT* "originalURL()" to return "askjskdhf.asfjhsdjfhsd" in the error case, right?

If so, what I suggested won't work.  Hmmm....

[Antonio Gomes]

thanks again for the comments , brady.

> the Mac bots are all running green right now, I assume you mean there's a Qt
> failure that this cleans up.  But have you tried running the Mac tests with
> your patch in?

I will try it at work tomorrow and report the results.

> I don't know - Do you have a Mac machine to try with your patch? 
> It's quite possible there isn't, as one thing I know this patch breaks - which
> is the WebKit API [WebDataSource unreachableURL] - has historically been
> difficult to test automatically.
> 
> If your patch *doesn't* break any Mac layout tests, then we need to take the
> time to add one.

that specific test would be definitively a great and needed add. So if no failure are catch on Mac when i re-run the tests, should I file a bug on adding that test ?

> > so how do access the documentLoader that actually refers to the latest load
> > (being in successful or not) ?
> 
> I followed your description here and looked at the other bug.  Amusingly, the
> main thing this patch would break is what happens in the error case, which
> covered by DocumentLoader::unreachableURL().

iirc, m_unreachebleURL will get set at object construction time. However I can not rely on it from qtwebkit api code, I am afraid.

> When you say "latest load", you are overloading some highly overloaded
> terminology.  In FrameLoader/DocumentLoader-land, the "current load" is the
> most recent main resource load to have been committed.  When a load fails
> because the url wasn't even valid, it was never committed, so the current
> activeDocumentLoader() remains unchanged.

sorry for mis-using the term ... i see and agree w/ what you pointed out, although i still think having access to the unsuccessful resource loader could be helpful. What do you think ?

> Alas, information about this attempted-load-not-even-getting-started is tacked
> on the DocumentLoader.  Based on reading the other bug, you are happy with
> url() and originalURL(), but just want to know about failure cases.  

exactly that.

> What happens if you try using activeDocumentLoader()->mainDocumentError()?

as you said, I could use that *if* activeDocumentLoader() returns the resource loader corresponding to the failing loader. I will check ...

[Brady Eidson]

> > What happens if you try using activeDocumentLoader()->mainDocumentError()?
> 
> as you said, I could use that *if* activeDocumentLoader() returns the resource
> loader corresponding to the failing loader. I will check ...

I don't have time to run the experiment right now, but I think mainResourceError might be set on the current document loader, even in this case, along with unreachableURL.  

I might be wrong, though.  Let us know what you find.

[Antonio Gomes]

(In reply to comment #7)
> > > What happens if you try using activeDocumentLoader()->mainDocumentError()?
> > 
> > as you said, I could use that *if* activeDocumentLoader() returns the resource
> > loader corresponding to the failing loader. I will check ...
> 
> I don't have time to run the experiment right now, but I think
> mainResourceError might be set on the current document loader, even in this
> case, along with unreachableURL.  

hum, DocumentLoader::setMainResourceError() gets called in the right (failing) loader, ok. It calls FrameLoader::setMainDocumentError(error), which on its turn calls FrameLoaderClientXXX()::setMainDocumentError ...

that can be possibly a bridge. I could try emit a signal from it to be catch in QWebFrame, where I need to know if the load was successful or not.

if it is not successful (case where I am stuck on) I can get the originalUrl from calling FrameLoader::outgoingReferrer()

simon, what do you think ?

[Antonio Gomes]

> > I don't know - Do you have a Mac machine to try with your patch? 
> > It's quite possible there isn't, as one thing I know this patch breaks - which
> > is the WebKit API [WebDataSource unreachableURL] - has historically been
> > difficult to test automatically.
> > 
> > If your patch *doesn't* break any Mac layout tests, then we need to take the
> > time to add one.

i will try it on a MAC when i have a change. and file a bug is no test catches the possibly problem introduced by my patch.

for now, closing this one as INVALID.