|Summary:||Cached main resources report a zero identifer on 304s|
|Product:||WebKit||Reporter:||Nate Chapin <japhet>|
|Component:||WebCore Misc.||Assignee:||Nate Chapin <japhet>|
|Severity:||Normal||CC:||abarth, buildbot, dfarler, jberlin, koivisto, rniwa, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:||105667|
Description Nate Chapin 2013-01-30 16:23:59 PST
Because of how MainResourceLoader tracks resource load identifiers and emulates them for MemoryCache hits, it gets into an inconsistent case in the 304 case. Namely, it no longer has an underlying ResourceLoader, but it also has an m_identifierForLoadWithoutResourceLoader equal to 0. I believe the solution is to check m_identifierForLoadWithoutResourceLoader instead of !loader() to determine whether MainResourceLoader needs to synthesize resource load callbacks. Patch shortly.
Comment 2 Adam Barth 2013-01-31 10:56:49 PST
Comment on attachment 185615 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=185615&action=review > Source/WebCore/ChangeLog:9 > + Tested manually. Among other things, the inspector will crash on a main resource 304, > + becuase it will have a garbage request id. Is there no way to test this automatically (or example with an inspector test)?
Comment 3 Nate Chapin 2013-01-31 11:11:09 PST
(In reply to comment #2) > (From update of attachment 185615 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185615&action=review > > > Source/WebCore/ChangeLog:9 > > + Tested manually. Among other things, the inspector will crash on a main resource 304, > > + becuase it will have a garbage request id. > > Is there no way to test this automatically (or example with an inspector test)? I'll see if I can find something. I rushed this out yesterday and didn't look very hard for a test.
Comment 4 Jessie Berlin 2013-01-31 12:03:33 PST
See also my comments in https://bugs.webkit.org/show_bug.cgi?id=108380 about tests that started failing recently that could be related
Comment 5 Ryosuke Niwa 2013-01-31 12:55:16 PST
I'm testing the patch now.
Comment 6 Ryosuke Niwa 2013-01-31 12:57:10 PST
I'll note that at least on Apple's internal performance test suite, this bug reproduces on different tests every time I run it and only after loading dozens of pages. So I'm having a hard time creating a reduction for it.
Comment 8 Build Bot 2013-01-31 15:14:31 PST
Comment on attachment 185871 [details] + test Attachment 185871 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16296042 New failing tests: http/tests/security/cross-origin-plugin-private-browsing-toggled.html
Comment 9 Adam Barth 2013-02-01 10:46:47 PST
Comment on attachment 185871 [details] + test Thanks for the test. I agree with Nate that the mac-wk2 failure is likely a flake since the same code change passed fine the last time.
Comment 10 WebKit Review Bot 2013-02-01 10:51:55 PST
Comment on attachment 185871 [details] + test Rejecting attachment 185871 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 185871, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: /mac/TestExpectations.rej patching file LayoutTests/platform/chromium/TestExpectations Hunk #1 succeeded at 4323 (offset -17 lines). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebCore/loader/MainResourceLoader.cpp patching file Source/WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/16305878
Comment 11 Nate Chapin 2013-02-01 11:04:50 PST
Created attachment 186081 [details] Patch for landing
Comment 12 WebKit Review Bot 2013-02-01 11:27:11 PST
Comment on attachment 186081 [details] Patch for landing Clearing flags on attachment: 186081 Committed r141615: <http://trac.webkit.org/changeset/141615>
Comment 13 WebKit Review Bot 2013-02-01 11:27:16 PST
All reviewed patches have been landed. Closing bug.