Bug 47736 - WebCore cache gets corrupted if revalidation request starts at an inopportune time
: WebCore cache gets corrupted if revalidation request starts at an inopportune...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Alexey Proskuryakov
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-15 13:30 PDT by Alexey Proskuryakov
Modified: 2010-10-15 17:42 PDT (History)
4 users (show)

See Also:


Attachments
proposed fix (71.14 KB, patch)
2010-10-15 13:38 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2010-10-15 13:30:55 PDT
If a revalidation request cannot be started due to SubresourceLoader::create() returning 0, we don't correctly cancel revalidation. So, a revalidation request with an error bit set gets stuck in cache.

<rdar://problem/8429396>
Comment 1 Alexey Proskuryakov 2010-10-15 13:38:48 PDT
Created attachment 70892 [details]
proposed fix
Comment 2 Darin Adler 2010-10-15 14:51:35 PDT
Comment on attachment 70892 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=70892&action=review

I am finding this a little hard to review because of the logging. Can we land an initial patch that just adds the logging, and then land the bug fix separately?

> WebCore/ChangeLog:6
> +        <rdar://problem/8429396> WebCore cache gets corrupted if revalidation request starts at a wrong time

I would call this “an inopportune time” maybe. It’s not “wrong”, is it?

> WebCore/loader/loader.cpp:387
> +            if (resource->resourceToRevalidate())
> +                cache()->revalidationFailed(resource);

Is this the bug fix?
Comment 3 Alexey Proskuryakov 2010-10-15 14:59:33 PDT
> I would call this “an inopportune time” maybe.

A word a day :-)

> > WebCore/loader/loader.cpp:387
> > +            if (resource->resourceToRevalidate())
> > +                cache()->revalidationFailed(resource);
> 
> Is this the bug fix?

Yes. I'll land it separately.
Comment 4 Alexey Proskuryakov 2010-10-15 15:14:19 PDT
Committed <http://trac.webkit.org/changeset/69887>, and logging in <http://trac.webkit.org/changeset/69886>.
Comment 5 WebKit Review Bot 2010-10-15 16:48:59 PDT
http://trac.webkit.org/changeset/69886 might have broken Leopard Intel Debug (Tests)
The following tests are not passing:
http/tests/uri/utf8-path.html
Comment 6 WebKit Review Bot 2010-10-15 16:49:09 PDT
http://trac.webkit.org/changeset/69887 might have broken Leopard Intel Debug (Tests)
The following tests are not passing:
http/tests/uri/utf8-path.html
Comment 7 Alexey Proskuryakov 2010-10-15 16:57:51 PDT
This sounds like it could be caused by the logging patch, but I don't see how exactly. I cannot reproduce this on Snow Leopard.
Comment 8 Alexey Proskuryakov 2010-10-15 17:19:49 PDT
OK, I got a stack trace from buildbot. Will fix (not yet sure how, but I will).
Comment 9 Alexey Proskuryakov 2010-10-15 17:42:54 PDT
This is a CRASH in FastMalloc, called from String::latin1() via CString::newUninitialized().

Committed speculative fix in <http://trac.webkit.org/changeset/69898>.