I’ll fix this soon, probably tonight. For now, turning off the test.
Mostly done with the patch for this. It’s the assertion that seems to be incorrect, not the code.
It’s hard to find real multipart mixed replace image tests on the web. I could use help with that!
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.
Created attachment 241103 [details] Patch
Created attachment 241104 [details] Patch
I also used http://www.fortnet.org/FortNet/HTML/Presentation/animation/pull2.html for some testing.
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 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.
Committed r175740: <http://trac.webkit.org/changeset/175740>