MHTMLParser does not set main resource when it has malformed Content-Type. So, m_archive->mainResource() can be NULL.
Created attachment 186601 [details] Patch
Created attachment 186602 [details] Patch Add missing files.
Comment on attachment 186602 [details] Patch Attachment 186602 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16371745 New failing tests: mhtml/page_with_malformed_content_type.mht
May I add empty pixel expectation for this test? In this test, I can not make testRunner.dumpAsText() call.
Created attachment 186620 [details] Patch Add pixel expectation.
Comment on attachment 186620 [details] Patch Perhaps we can use a ref test?
(In reply to comment #6) > (From update of attachment 186620 [details]) > Perhaps we can use a ref test? Thank you for your idea. May I use an empty html file for ref?
Created attachment 186786 [details] Patch
Created attachment 186795 [details] Patch
Comment on attachment 186795 [details] Patch Attachment 186795 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16396124
Created attachment 186972 [details] Patch Re-upload patch again because of win-ews false alarm.
(In reply to comment #11) > Created an attachment (id=186972) [details] > Patch > > Re-upload patch again because of win-ews false alarm. cr-android-ews :(
Created attachment 187262 [details] Patch
Review?
Is this patch still needed?
Seems like it's needed for MHTML support, but I'm not sure if any ports enable MHTML. Perhaps the whole feature should be removed? ENABLE_MHTML is never seen in sources, but it's in Tools/Scripts/webkitperl/FeatureList.pm: define => "ENABLE_MHTML", default => (isGtk() || isEfl()), value => \$mhtmlSupport }, What is this file used for? Does this mean that Gtk and Efl enable MHTML?
(In reply to comment #16) > Seems like it's needed for MHTML support, but I'm not sure if any ports enable MHTML. Perhaps the whole feature should be removed? > > ENABLE_MHTML is never seen in sources, but it's in Tools/Scripts/webkitperl/FeatureList.pm: > > define => "ENABLE_MHTML", default => (isGtk() || isEfl()), value => \$mhtmlSupport }, > > What is this file used for? Does this mean that Gtk and Efl enable MHTML? I'm not sure what that file is used for, but it is in some cmake build files as being turned on for EFL and Gtk: Source/cmake/OptionsEfl.cmake 80:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MHTML ON) Source/cmake/OptionsGTK.cmake 52:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MHTML ON) But it doesn't answer if they're actually using it in practice.
(In reply to comment #17) > (In reply to comment #16) > > Seems like it's needed for MHTML support, but I'm not sure if any ports enable MHTML. Perhaps the whole feature should be removed? > > > > ENABLE_MHTML is never seen in sources, but it's in Tools/Scripts/webkitperl/FeatureList.pm: > > > > define => "ENABLE_MHTML", default => (isGtk() || isEfl()), value => \$mhtmlSupport }, > > > > What is this file used for? Does this mean that Gtk and Efl enable MHTML? > > I'm not sure what that file is used for, but it is in some cmake build files as being turned on for EFL and Gtk: > > Source/cmake/OptionsEfl.cmake > 80:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MHTML ON) > > Source/cmake/OptionsGTK.cmake > 52:WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MHTML ON) > > But it doesn't answer if they're actually using it in practice. Gyuyoung should confirm but last I checked we were using MHTML. FYI, I believe I fixed this crash (and another one iirc) on Blink side: https://src.chromium.org/viewvc/blink?view=rev&revision=164584
Comment on attachment 187262 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187262&action=review > Source/WebCore/loader/DocumentLoader.cpp:498 > return false; If you clear m_archive here before returning, then you no longer need the fix in DocumentLoader::commitData(). See corresponding Blink fix: https://src.chromium.org/viewvc/blink?revision=164584&view=revision
Created attachment 253191 [details] Patch
Comment on attachment 253191 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253191&action=review I am OK with this change, but I think we can do better. > Source/WebCore/loader/DocumentLoader.cpp:970 > + if (!m_archive || !m_archive->mainResource()) { I suggest we make this fix inside ArchiveFactory::create. That function should never return an archive without a main resource. Further, the Archive class should be changed to have a reference for the main resource instead of a pointer. We don’t need to be able to create archives without main resources.