Bug 108402

Summary: Cached main resources report a zero identifer on 304s
Product: WebKit Reporter: Nate Chapin <japhet>
Component: WebCore Misc.Assignee: Nate Chapin <japhet>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, buildbot, dfarler, jberlin, koivisto, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 105667    
Bug Blocks: 108380    
Attachments:
Description Flags
patch
none
+ test
none
Patch for landing none

Nate Chapin
Reported 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.
Attachments
patch (2.23 KB, patch)
2013-01-30 16:27 PST, Nate Chapin 		no flags
DetailsFormatted DiffDiff
+ test (7.66 KB, patch)
2013-01-31 14:49 PST, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.96 KB, patch)
2013-02-01 11:04 PST, Nate Chapin 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Nate Chapin
Comment 1 2013-01-30 16:27:34 PST
Created attachment 185615 [details] patch
Adam Barth
Comment 2 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)?
Nate Chapin
Comment 3 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.
Jessie Berlin
Comment 4 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
Ryosuke Niwa
Comment 5 2013-01-31 12:55:16 PST
I'm testing the patch now.
Ryosuke Niwa
Comment 6 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.
Nate Chapin
Comment 7 2013-01-31 14:49:45 PST
Created attachment 185871 [details] + test
Build Bot
Comment 8 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
Adam Barth
Comment 9 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.
WebKit Review Bot
Comment 10 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
Nate Chapin
Comment 11 2013-02-01 11:04:50 PST
Created attachment 186081 [details] Patch for landing
WebKit Review Bot
Comment 12 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>
WebKit Review Bot
Comment 13 2013-02-01 11:27:16 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.