Bug 138358

Summary: REGRESSION (r175549): http/tests/multipart/stop-crash.html failing (assertion failure in CachedResource::setEncodedSize)
Product: WebKit Reporter: Darin Adler <darin>
Component: Page LoadingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, japhet
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch ap: review+, ap: commit-queue-

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>