Bug 94644

Summary: Failure to dispatch delegate callbacks if resource load fails synchronously
Product: WebKit Reporter: Pratik Solanki <psolanki>
Component: Page LoadingAssignee: Pratik Solanki <psolanki>
Status: REOPENED ---    
Severity: Normal CC: abarth, ap, cmarcelo, japhet, kbr, koivisto, macpherson, menard, psolanki, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch koivisto: review+

Description Pratik Solanki 2012-08-21 16:17:18 PDT
This is similar to bug 91018, but instead of special casing Font resources, we should generalize it so that this can't happen for other resource load fails.
Comment 1 Pratik Solanki 2012-08-21 16:17:54 PDT
<rdar://problem/12097409>
Comment 2 Pratik Solanki 2012-08-21 16:27:35 PDT
Created attachment 159793 [details]
Patch
Comment 3 Antti Koivisto 2012-08-21 22:47:07 PDT
Comment on attachment 159793 [details]
Patch

r=me. A test case would be cool.
Comment 4 Pratik Solanki 2012-08-22 11:44:25 PDT
Committed r126325: <http://trac.webkit.org/changeset/126325>
Comment 5 Kenneth Russell 2012-08-22 17:34:27 PDT
I'm afraid this introduced a subtle regression that, bizarrely enough, only showed up in Chromium Mac debug builds with the layout test svg/custom/linking-uri-01-b.svg . The failure is 100% reproducible on my Mac Pro with a debug build of Chromium's DumpRenderTree, and reverting this patch locally fixes the regression. The bug is that with this test case, the onload handler isn't being called at the right time, so the ellipse isn't zoomed to fill the view. I wasn't able to reproduce the failure on Linux and it looks like the Apple bots aren't affected either.

I'm concerned that this indicates a change in behavior that will affect real web sites and therefore intend to roll out this patch. Pratik, I'll try to reach you on IRC before doing so.

First failing builds:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28dbg%29/builds/12059
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.7%20%28dbg%29/builds/54

Flakiness dashboard:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2Fcustom%2Flinking-uri-01-b.svg
Comment 6 Kenneth Russell 2012-08-22 17:46:26 PDT
Adding a couple more people who know the loader code. Changes in behavior in this area are really scary, so unless I can reach Pratik soon, I'll roll this out.

Haven't had any luck yet identifying new layout test failures on the Apple Mac debug bots caused by this change, but still looking.
Comment 7 Kenneth Russell 2012-08-22 18:04:27 PDT
Pratik and I talked on IRC and he agreed that it would be OK to roll out this patch. I spent some time going through the Apple Mac test bots on build.webkit.org around the revision where this landed. The only evidence I could find that something might be going wrong on other ports is that the following bot started getting an elevated crash rate at r126325 (2x the previous crash rate, from 12 to 24) -- and it has a build (#2757) where only that revision was incorporated:

http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK2%20%28Tests%29?numbuilds=25

Again, though, this change definitely affected the timing of the calling of the onload hander in the SVG layout test above in Chromium Mac Debug builds. I'm available to help test revisions to the patch.
Comment 8 Kenneth Russell 2012-08-22 18:06:59 PDT
Reverted r126325 for reason:

Caused subtle but reproducible failure to call onload handler properly in an SVG layout test in Chromium Mac Debug builds, indicating potentially larger problem

Committed r126373: <http://trac.webkit.org/changeset/126373>