Cached main resources report a zero identifer on 304s

[Nate Chapin]

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.

[Nate Chapin]

Created attachment 185615 [details]
patch

[Adam Barth]

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]

(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]

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]

I'm testing the patch now.

[Ryosuke Niwa]

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]

Created attachment 185871 [details]
+ test

[Build Bot]

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 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 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]

Created attachment 186081 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 186081 [details]
Patch for landing

Clearing flags on attachment: 186081

Committed r141615: <http://trac.webkit.org/changeset/141615>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.