Bug 138358 – REGRESSION (r175549): http/tests/multipart/stop-crash.html failing (assertion failure in CachedResource::setEncodedSize)

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Adler]

I’ll fix this soon, probably tonight. For now, turning off the test.

[Darin Adler]

Mostly done with the patch for this. It’s the assertion that seems to be incorrect, not the code.

[Darin Adler]

It’s hard to find real multipart mixed replace image tests on the web. I could use help with that!

[Alexey Proskuryakov]

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.

[Darin Adler]

Created attachment 241103 [details]
Patch

[Darin Adler]

Created attachment 241104 [details]
Patch

[Darin Adler]

I also used http://www.fortnet.org/FortNet/HTML/Presentation/animation/pull2.html for some testing.

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Darin Adler]

Committed r175740: <http://trac.webkit.org/changeset/175740>