27444 2009-07-20 06:35:23 -0700 Wrong FrameLoader::activeDocumentLoader when loading an invalid URL. 2009-07-25 08:29:57 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) PC Linux RESOLVED INVALID P2 Normal --- 25867 0 tonikitoo webkit-unassigned andersca beidson hausmann oldest_to_newest 133122 0 33081 tonikitoo 2009-07-20 06:35:23 -0700 Created attachment 33081 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 133192 1 tonikitoo 2009-07-20 12:11:52 -0700 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. 133212 2 33081 beidson 2009-07-20 13:37:50 -0700 Comment on attachment 33081 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. 133218 3 tonikitoo 2009-07-20 13:54:58 -0700 > 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. 133229 4 beidson 2009-07-20 14:22:38 -0700 (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()? 133230 5 beidson 2009-07-20 14:24:25 -0700 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.... 133327 6 tonikitoo 2009-07-20 19:03:15 -0700 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 ... 133341 7 beidson 2009-07-20 19:44:55 -0700 > > 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. 133471 8 tonikitoo 2009-07-21 08:39:42 -0700 (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 ? 134742 9 tonikitoo 2009-07-25 08:29:57 -0700 > > 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. 33081 2009-07-20 06:35:23 -0700 2009-07-20 13:37:49 -0700 set the provisional documentLoader as the activeDocument in case of load failures (v0.1) BUG_XXX.diff text/plain 705 tonikitoo ZGlmZiAtLWdpdCBhL1dlYkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcCBiL1dlYkNvcmUvbG9h ZGVyL0ZyYW1lTG9hZGVyLmNwcAppbmRleCBhZjU5Mjk5Li4zNThiMmQ5IDEwMDY0NAotLS0gYS9X ZWJDb3JlL2xvYWRlci9GcmFtZUxvYWRlci5jcHAKKysrIGIvV2ViQ29yZS9sb2FkZXIvRnJhbWVM b2FkZXIuY3BwCkBAIC00ODcyLDYgKzQ4NzIsMTAgQEAgdm9pZCBGcmFtZUxvYWRlcjo6bWFpblJl Y2VpdmVkQ29tcGxldGVFcnJvcihEb2N1bWVudExvYWRlciogbG9hZGVyLCBjb25zdCBSZXNvdXIK IHsKICAgICBsb2FkZXItPnNldFByaW1hcnlMb2FkQ29tcGxldGUodHJ1ZSk7CiAgICAgbV9jbGll bnQtPmRpc3BhdGNoRGlkTG9hZE1haW5SZXNvdXJjZShhY3RpdmVEb2N1bWVudExvYWRlcigpKTsK KyAgICAvLyBzZXR0aW5nIHRoZSBwcm92aXNpb25hbERvY3VtZW50TG9hZGVyIGFzIGFjdGl2ZURv Y3VtZW50TG9hZGVyIHNvCisgICAgLy8gd2UgaGF2ZSBhIHJlZmVyZW5jZSBmb3IgdGhlIGFjdGl2 ZSBsb2FkZXIgYWZ0ZXIgdGhlIGxvYWRpbmcKKyAgICAvLyBmYWlsdXJlIGhhcHBlbmRlZC4KKyAg ICBzZXREb2N1bWVudExvYWRlcihtX3Byb3Zpc2lvbmFsRG9jdW1lbnRMb2FkZXIuZ2V0KCkpOwog ICAgIGNoZWNrQ29tcGxldGVkKCk7CiAgICAgaWYgKG1fZnJhbWUtPnBhZ2UoKSkKICAgICAgICAg Y2hlY2tMb2FkQ29tcGxldGUoKTsK