Don't wait for finished replies #428

Merged
merged 3 commits into from Dec 6, 2012

4 participants

@mhoran

Synchronous requests will have emitted the finished signal by the time
we connect to it.

@mhoran mhoran Don't wait for finished replies
Synchronous requests will have emitted the finished signal by the time
we connect to it.
5bef336
@lucassus

Unfortunately it still does not work for me.

webkit_debug gives me the following output:

Received 200 from "https://ajax.googleapis.com/ajax/libs/jquery/1.6/jquery.min.js" 
2 requests remaining 
Page finished with true 
Started request to "https://js.appcenter.intuit.com/Content/IA/intuit.ipp.anywhere.css" 
Started request to "https://appcenter.intuit.com/trackapi/TrackingActions?remote=true&pn=Activecell%20%7C%20The%20small%20business%20management%20platform&nm=iaLoginButtonHorizontal&typ=Render&txt=SignInWithIntuit&et=load&ref=&url=http%3A%2F%2F127.0.0.1%3A39593%2Fusers%2Fsign_in%3F_subdomain%3Dlaunchpad&host=127.0.0.1%3A39593&callback=jQuery164030159705178812146_1354612306948&_=1354612306953" 
Received 200 from "https://js.appcenter.intuit.com/Content/IA/intuit.ipp.anywhere.css" 
3 requests remaining 
Started request to "https://js.appcenter.intuit.com/Content/IA/button_signinwithintuit_horiz_small.png" 
Received 200 from "https://js.appcenter.intuit.com/Content/IA/button_signinwithintuit_horiz_small.png" 
3 requests remaining 
Received 200 from "https://appcenter.intuit.com/trackapi/TrackingActions?remote=true&pn=Activecell | The small business management platform&nm=iaLoginButtonHorizontal&typ=Render&txt=SignInWithIntuit&et=load&ref=&url=http://127.0.0.1:39593/users/sign_in?_subdomain=launchpad&host=127.0.0.1:39593&callback=jQuery164030159705178812146_1354612306948&_=1354612306953" 
2 requests remaining

Sometimes in the last line I have Page finished with true.
With the previous capybara-webkit version everything works fine. It's strange, because I use the most recent version of capybara-webkit in one of my open source project (https://github.com/lucassus/mongo_browser/tree/master/spec/features) and everything works fine.

Here is how my Gemfile.lock looks: https://gist.github.com/985d54f7eb2558f453a9

@mhoran

Thanks for your feedback. I think there's still a possible race condition here. Do you happen to be doing anything that could be opening a new window?

@lucassus

I don't think so, the race condition by definition is sth unpredictable.
I have this result every time time I execute the spec.

@ryreitsma

FYI: we experienced the issue that Capybara hangs unexpectedly after a multipart fileupload and a page refresh, using some nifty javascript. This actually fixes our issue, so great work, thanks.

@jferris jferris commented on the diff Dec 4, 2012
src/WebPageManager.cpp
@@ -51,7 +49,18 @@ void WebPageManager::emitLoadStarted() {
void WebPageManager::requestCreated(QByteArray &url, QNetworkReply *reply) {
logger() << "Started request to" << url;
- m_started += reply;
+ if (reply->isFinished())
@jferris
thoughtbot, inc. member
jferris added a note Dec 4, 2012

Is there a race condition here? Is it possible that we get a false from this method and then it finishes before we call connect below? It's not clear to me how much of the reply behavior is asynchronous.

Another way to handle this would be to remove finished replies when we get the page load signal. Currently, the logic is "are there any replies left;" we can change that to "are there any unfinished replies left."

@mhoran
mhoran added a note Dec 4, 2012

I think that's a possibility. We've used this pattern elsewhere, so I went with it.

I believe the reply in progress logic was added for multi-window sync. However, it probably makes more sense to check whether there are any windows loading instead of any replies, since the pageLoading signal is more reliable. This would probably resolve the other deadlocks as well.

@jferris
thoughtbot, inc. member
jferris added a note Dec 4, 2012

I originally changed this to check replies for a few reasons:

  • Multi-window sync
  • iframes
  • It now waits for dependent assets, including JavaScript

Unfortunately, the pageLoading signal is just differently reliable; for example, you'll get multiple signals for a page with iframes, and the number of start signals doesn't always equal the number of finished signals. I think we're going to need to deduce the actual page state based on a combination of reply and page signals. That's sort of what we're doing now, but there are clearly holes in the existing logic.

Let's merge what you have; we can change the way we handle replies in a later commit if we need to. The UnsupportedContent changes are nice, and this at least reduces the possibility of a deadlock.

Related: I'd like to build up a suite of tests that reproduce these kinds of issues, similar to the malloc_spec we used for finding the segfault in 4.8. That way, we have tests we can run before release that reduce the possibility of reintroducing race conditions. I'll give that a shot this week.

Anyway, nice work on finding the finished signal edge case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mhoran

@lucassus, could paste the full debug output of the failing scenario? I was debugging this tonight and noticed that if some resources are requested over HTTP and others over HTTPS, WebKit seems to hang, likely for security reasons. It looks like the jquery request is hanging, but I'm not sure what else.

I've not been able to come up with a failing scenario for your case. I'm not sure if @jferris has had luck; he mentioned something in one of the related issues.

@mhoran mhoran and 1 other commented on an outdated diff Dec 5, 2012
src/UnsupportedContentHandler.cpp
@@ -10,8 +10,7 @@
void UnsupportedContentHandler::renderNonHtmlContent() {
QByteArray text = m_reply->readAll();
m_page->mainFrame()->setContent(text, QString("text/plain"), m_reply->url());
- m_page->setUnsupportedContentLoaded();
- m_page->networkAccessManagerFinishedReply(m_reply);
+ m_page->unsupportedContentFinishedReply(m_reply);
m_page->loadFinished(true);
@mhoran
mhoran added a note Dec 5, 2012

If we leave the loadFinished signal connected to the page, this will happen automatically. Any reason this was disconnected?

@jferris
thoughtbot, inc. member
jferris added a note Dec 5, 2012

I can't remember.

We went through a few iterations with the unsupported content stuff before we got one that worked right. Issues I remember:

  • Multiple loadFinished signals
  • No loadFinished signals
  • loadFinished signal with a false parameter (meaning that it failed)

I don't think there were any race conditions here, so I think you just need to get the tests passing. The commit log may know more.

@mhoran
mhoran added a note Dec 5, 2012

The tests pass without with the signal hooked up and the line removed. There was a bunch of rework leading up to what we have now, so it's possible that we can just leave the signal hooked up and remove the line.

@jferris
thoughtbot, inc. member
jferris added a note Dec 5, 2012

Okay - if we don't know why the code is there, and the tests pass, let's remove it. I don't want to have dead code slowing us down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mhoran

@jferris, we don't ever seem to get inside http://git.io/ZKtWlQ. This looks to be a relic from this refactoring: http://git.io/EmaD7A. This means we only emit pageFinished from setPageStatus. I'm wondering if there's a situation where some outgoing requests may be sitting around in m_started, unrelated to the current request. Because they're never removed, and may never finish, we incorrectly assume that the WebPage is still loading.

I'm wondering if a simpler solution would be to catch all the loadStarted signals and only emit pageFinished when they all finish. If we keep them in a QSet (like we do with m_started), wouldn't that take care of duplicate loadStarted events?

I also think some of the confusing behavior regarding event firing may have been resolved by http://git.io/e_NaMg. This ensures that all pending events have processed by the time we check whether or not the page is loading.

@lucassus

@mhoran here is the full debug output from my hanging scenario https://gist.github.com/4218354

@jferris
thoughtbot, inc. member

@mhoran that sounds fine to me if it works. I've definitely run into situations where the number of loadStarted signals wasn't the same as the number of pageFinished signals. Also, is there an object that represents a "page load?" How will we track which loads are ongoing if they don't have a unique reference?

mhoran added some commits Dec 5, 2012
@mhoran mhoran Emit loadStarted and pageFinished on WebPage state change
Instead of emitting loadStarted when a reply is created and pageFinished
when all pending replies have finished, emit these signals when the
corresponding WebPage events fire.
b28452e
@mhoran mhoran Don't disconnect loadFinished in UnsupportedContentHandler
WebPage::setContent will cause the loadFinished signal to fire, so
there's no need to call loadFinished if the signal is connected.
2eb38b3
@mhoran

@lucassus, could you give the latest changes on this branch a shot? I've changed the logic to not care about whether or not all replies have finished but instead depend on whether or not the WebPage has finished loading

@jferris, let me know your thoughts regarding b28452e. I'm not sure if this approach wasn't working for you before, or if I'm missing something. However, the test suite is passing reliably for me (I even ran it on Windows!)

@lucassus

@mhoran it was tested on this version mhoran@5bef336

@mhoran

@lucassus, I believe b28452e may be relevant to you. If you've got the git requirement in your gemfile, you should be able to bundle update capybara-webkit to get the latest.

@jferris
thoughtbot, inc. member

@mhoran I think that's worth a try. Looks good to merge.

@mhoran mhoran merged commit 2eb38b3 into thoughtbot:master Dec 6, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment