Bug 138358 - REGRESSION (r175549): http/tests/multipart/stop-crash.html failing (assertion failure in CachedResource::setEncodedSize)
Summary: REGRESSION (r175549): http/tests/multipart/stop-crash.html failing (assertion...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-04 08:56 PST by Darin Adler
Modified: 2014-11-06 19:16 PST (History)
3 users (show)

See Also:


Attachments
Patch (5.18 KB, patch)
2014-11-06 07:15 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (5.22 KB, patch)
2014-11-06 07:19 PST, Darin Adler
ap: review+
ap: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2014-11-04 08:56:53 PST
I’ll fix this soon, probably tonight. For now, turning off the test.
Comment 1 Darin Adler 2014-11-05 09:24:15 PST
Mostly done with the patch for this. It’s the assertion that seems to be incorrect, not the code.
Comment 2 Darin Adler 2014-11-05 09:24:46 PST
It’s hard to find real multipart mixed replace image tests on the web. I could use help with that!
Comment 3 Alexey Proskuryakov 2014-11-05 09:53:59 PST
We usually learn about these through bug reports about broken webcams, which go away after a bug is fixed.

rdar://problem/12369833 has a URL that is currently live.

http://158.250.33.102/axis-cgi/mjpg/video.cgi?camera=1&1415209935808 is an example I found by searching the web now.
Comment 4 Darin Adler 2014-11-06 07:15:18 PST
Created attachment 241103 [details]
Patch
Comment 5 Darin Adler 2014-11-06 07:19:53 PST
Created attachment 241104 [details]
Patch
Comment 6 Darin Adler 2014-11-06 07:20:35 PST
I also used http://www.fortnet.org/FortNet/HTML/Presentation/animation/pull2.html for some testing.
Comment 7 Alexey Proskuryakov 2014-11-06 10:04:27 PST
Comment on attachment 241104 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        (WebCore::DocumentLoader::subresourceLoaderFinishedLoadingOnePart): Fix another
> +        incorrect assertion that can fire for any parts after the first one. I ran into

This change looks like it changes behavior in release mode, too - we used to call remove() unconditionally, but no longer do.

> Source/WebCore/ChangeLog:18
> +        this while testing various websites that use multipart/mixed-replace.

Is there a way to add a regression test for that? Sounds like this issue was not covered by existing tests.

> Source/WebCore/loader/DocumentLoader.cpp:1494
> +    auto identifier = loader->identifier();

I wanted to know what "identifier" was, but with auto, I can't see that.

> Source/WebCore/loader/cache/CachedResource.cpp:-531
> -    // The size cannot ever shrink (unless it is being nulled out because of an error).  If it ever does, assert.
> -    ASSERT(size == 0 || size >= m_encodedSize);

Presumably we don't actually ever cache parts of a multipart response, and CachedResource is just a somewhat misleading class name.

> LayoutTests/TestExpectations:233
> +webkit.org/b/138145 fast/multicol/multicol-crazy-nesting.html [ Skip ]

This doesn't look like part of this change.
Comment 8 Darin Adler 2014-11-06 19:07:41 PST
Comment on attachment 241104 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        incorrect assertion that can fire for any parts after the first one. I ran into
> 
> This change looks like it changes behavior in release mode, too - we used to call remove() unconditionally, but no longer do.

That’s true. If we wanted to be more conservative I could trivially change the patch to call remove unconditionally.

>> Source/WebCore/ChangeLog:18
>> +        this while testing various websites that use multipart/mixed-replace.
> 
> Is there a way to add a regression test for that? Sounds like this issue was not covered by existing tests.

Maybe, not sure.

>> Source/WebCore/loader/DocumentLoader.cpp:1494
>> +    auto identifier = loader->identifier();
> 
> I wanted to know what "identifier" was, but with auto, I can't see that.

True; we also couldn’t see that in the old code.

>> Source/WebCore/loader/cache/CachedResource.cpp:-531
>> -    ASSERT(size == 0 || size >= m_encodedSize);
> 
> Presumably we don't actually ever cache parts of a multipart response, and CachedResource is just a somewhat misleading class name.

That’s right. Your question makes me realize that another way to fix this would be to add an early return for !inCache() that only sets m_encodedSize; then we could keep the assertion.

>> LayoutTests/TestExpectations:233
>> +webkit.org/b/138145 fast/multicol/multicol-crazy-nesting.html [ Skip ]
> 
> This doesn't look like part of this change.

Looks like I made a merge mistake.
Comment 9 Darin Adler 2014-11-06 19:16:36 PST
Committed r175740: <http://trac.webkit.org/changeset/175740>