146019 2015-06-16 08:21:20 -0700 WebProcess crashes after too many redirect error when there's an active NPAPI plugin 2015-06-19 10:58:47 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Soup P2 Normal --- 1 cgarcia webkit-unassigned andersca ap beidson commit-queue darin japhet oldest_to_newest 1102234 0 cgarcia 2015-06-16 08:21:20 -0700 #0 0x00007f17222e3be2 in WebCore::DocumentLoader::detachFromFrame() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #1 0x00007f17222f8521 in WebCore::FrameLoader::setProvisionalDocumentLoader(WebCore::DocumentLoader*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #2 0x00007f17222f8a4b in WebCore::FrameLoader::clearProvisionalLoad() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #3 0x00007f17222ff516 in WebCore::FrameLoader::checkLoadCompleteForThisFrame() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #4 0x00007f17222ff615 in WebCore::FrameLoader::checkLoadComplete() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #5 0x00007f17223001d0 in WebCore::FrameLoader::receivedMainResourceError(WebCore::ResourceError const&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #6 0x00007f172236e564 in WebCore::CachedResource::checkNotify() [clone .part.177] () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #7 0x00007f172233438c in WebCore::SubresourceLoader::didFail(WebCore::ResourceError const&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #8 0x00007f1721cc5b3b in void IPC::handleMessage<Messages::WebResourceLoader::DidFailResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::ResourceError const&)>(IPC::MessageDecoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::ResourceError const&)) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #9 0x00007f1721cc56ca in WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #10 0x00007f1721a2690b in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::MessageDecoder, std::default_delete<IPC::MessageDecoder> >) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #11 0x00007f1721a27304 in IPC::Connection::dispatchOneMessage() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #12 0x00007f1723041864 in WTF::RunLoop::performWork() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #13 0x00007f17203a6295 in WTF::GMainLoopSource::voidCallback() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #14 0x00007f17203a432a in WTF::GMainLoopSource::voidSourceCallback(WTF::GMainLoopSource*) () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18 #15 0x00007f171d49bbdd in g_main_dispatch (context=0x1c08900) at gmain.c:3122 #16 g_main_context_dispatch (context=context@entry=0x1c08900) at gmain.c:3737 #17 0x00007f171d49bf78 in g_main_context_iterate (context=0x1c08900, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3808 #18 0x00007f171d49c292 in g_main_loop_run (loop=0x22b3780) at gmain.c:4002 #19 0x00007f1723046ff0 in WTF::RunLoop::run() () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #20 0x00007f1721c95482 in WebProcessMainUnix () from /home/cgarcia/src/git/gnome/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37 #21 0x00007f17176e3b45 in __libc_start_main (main=0x400a90 <main>, argc=2, argv=0x7fff17260f98, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fff17260f88) at libc-start.c:287 #22 0x0000000000400ae5 in _start () 1102236 1 cgarcia 2015-06-16 08:34:11 -0700 This happens with the GTK+ port after a navigation action ends up in an infinite redirection and the ResourceHandle fails with too many redirections error. I should actually happen after any error is reported by the ResourceHnalder before the load is committed. But tt only happens if there's an active NPAPI plugin. The problem is that FrameLoader::receivedMainResourceError() is called recursively because DocumentLoader::stopLoading() ends up calling mainReceivedError() that calls FrameLoader::receivedMainResourceError() again. DocumentLoader::stopLoading() checks if the document is still loading, which can happen if the main resource is loading, if there's any subresource loading or if there's a plugin loading. So, in case of being loading, those cases are handled individually to cancel the main resource, or set an error in the document loader and cancel subresources and plugins, except for this case of plugins, that mainReceivedError is called instead of setting cancelled error on the document loader. 1102237 2 254951 cgarcia 2015-06-16 08:38:31 -0700 Created attachment 254951 Patch 1102246 3 darin 2015-06-16 09:38:51 -0700 Added Anders also. I think this patch is right but I am not sure. 1102261 4 254951 darin 2015-06-16 10:38:42 -0700 Comment on attachment 254951 Patch I read over the code again and I am more sure this is a correct change. Is there some way to make a regression test for this? That would really help for the future. 1102482 5 cgarcia 2015-06-16 22:37:03 -0700 (In reply to comment #4) > Comment on attachment 254951 [details] > Patch > > I read over the code again and I am more sure this is a correct change. > > Is there some way to make a regression test for this? That would really help > for the future. I thought about it, but I'm afraid it's not that easy to write a cross platform layout test for this. We would need a test with a NPAPI plugin running that produces a main resource load error before the load is committed. For soup based ports we could indeed use an infinite redirection to trigger too many redirections error, but I don't know if that would work in mac too. I tried with a GTK+ specific unit test and I was unable to reproduce the crash. And I can't share the website in which the crash is always reproducible :-( In any case, I can try to write a layout test for this if someone has an idea of how to reproduce this, and helps me a bit. 1102490 6 cgarcia 2015-06-16 23:04:15 -0700 Committed r185643: <http://trac.webkit.org/changeset/185643> 1102574 7 ap 2015-06-17 09:44:07 -0700 > We would need a test with a NPAPI plugin running that produces a main resource load error before the load is committed. We have a TestNetscapePlugin project that can be modified in any way we want it to. Most of the time, it's used without a main resource, but there is no reason why it has to stay that way. > For soup based ports we could indeed use an infinite redirection to trigger too many redirections error, but I don't know if that would work in mac too. An infinite redirection certainly triggers an error in CFNetwork. 1102577 8 cgarcia 2015-06-17 09:48:06 -0700 (In reply to comment #7) > > We would need a test with a NPAPI plugin running that produces a main resource load error before the load is committed. > > We have a TestNetscapePlugin project that can be modified in any way we want > it to. Most of the time, it's used without a main resource, but there is no > reason why it has to stay that way. Sure, I know TestNetscapePlugin :-) > > For soup based ports we could indeed use an infinite redirection to trigger too many redirections error, but I don't know if that would work in mac too. > > An infinite redirection certainly triggers an error in CFNetwork. And that's reported from ResourceHandle to the client via didFail()? Ok, I'll see if I can write a test, I guess it needs to be a http test. 1102580 9 ap 2015-06-17 10:05:05 -0700 > And that's reported from ResourceHandle to the client via didFail()? Correct, we get a didFail(). 1102883 10 cgarcia 2015-06-18 04:58:56 -0700 I'm trying to make the test, but I don't manage to make it work. I tried with the too many redirects error, first doing a client redirect to a php that makes the server redirect to itself. The problem is that as soon as the new main resource load starts due to the client direction, the plugin load is cancelled, and when the error happens in DocumentLoader::stopLoading(), the plugin is no longer loading. 1103271 11 darin 2015-06-19 10:24:04 -0700 Do you understand why that didn’t happen in the original case where you discovered the bug? 1103281 12 cgarcia 2015-06-19 10:36:33 -0700 (In reply to comment #11) > Do you understand why that didn’t happen in the original case where you > discovered the bug? No, I don't :-( It's a crappy vpn access system that uses an obscure NPAPI plugin. Everytime I clicked on disconnect button, the web process crashed, and then I debugged it and noticed it was after a too many redirections error. But I don't even know why the too many redirections error happens. I've tried to check the code with the inspector, but got lost in the HTML and JavaScript mess. Now, the web process doesn't crash, but of course I always get a too many redirections error when disconnecting from the vpn. 1103300 13 darin 2015-06-19 10:58:47 -0700 I hate to be the one to state this annoying fact, but it’s things like this where there is real value in figuring out how to create tests. I understand, though, if you get stuck and are unable to do it. 254951 2015-06-16 08:38:31 -0700 2015-06-16 10:38:42 -0700 Patch wcore-stop-loading-crash.diff text/plain 3531 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAzOTU2ZjRmLi4zMjgyOTdlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMzIg QEAKKzIwMTUtMDYtMTYgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29t PgorCisgICAgICAgIFdlYlByb2Nlc3MgY3Jhc2hlcyBhZnRlciB0b28gbWFueSByZWRpcmVjdCBl cnJvciB3aGVuIHRoZXJlJ3MgYW4gYWN0aXZlIE5QQVBJIHBsdWdpbgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ2MDE5CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBoYXBwZW5zIHdpdGggdGhlIEdU SysgcG9ydCBhZnRlciBhIG5hdmlnYXRpb24gYWN0aW9uIGVuZHMgdXAKKyAgICAgICAgaW4gYW4g aW5maW5pdGUgcmVkaXJlY3Rpb24gYW5kIHRoZSBSZXNvdXJjZUhhbmRsZSBmYWlscyB3aXRoIHRv bworICAgICAgICBtYW55IHJlZGlyZWN0aW9ucyBlcnJvci4gSSBzaG91bGQgYWN0dWFsbHkgaGFw cGVuIGFmdGVyIGFueSBlcnJvcgorICAgICAgICBpcyByZXBvcnRlZCBieSB0aGUgUmVzb3VyY2VI bmFsZGVyIGJlZm9yZSB0aGUgbG9hZCBpcworICAgICAgICBjb21taXR0ZWQuIEJ1dCB0dCBvbmx5 IGhhcHBlbnMgaWYgdGhlcmUncyBhbiBhY3RpdmUgTlBBUEkKKyAgICAgICAgcGx1Z2luLiBUaGUg cHJvYmxlbSBpcyB0aGF0IEZyYW1lTG9hZGVyOjpyZWNlaXZlZE1haW5SZXNvdXJjZUVycm9yKCkK KyAgICAgICAgaXMgY2FsbGVkIHJlY3Vyc2l2ZWx5IGJlY2F1c2UgRG9jdW1lbnRMb2FkZXI6OnN0 b3BMb2FkaW5nKCkgZW5kcyB1cAorICAgICAgICBjYWxsaW5nIG1haW5SZWNlaXZlZEVycm9yKCkg dGhhdCBjYWxscyBGcmFtZUxvYWRlcjo6cmVjZWl2ZWRNYWluUmVzb3VyY2VFcnJvcigpCisgICAg ICAgIGFnYWluLiBEb2N1bWVudExvYWRlcjo6c3RvcExvYWRpbmcoKSBjaGVja3MgaWYgdGhlIGRv Y3VtZW50IGlzCisgICAgICAgIHN0aWxsIGxvYWRpbmcsIHdoaWNoIGNhbiBoYXBwZW4gaWYgdGhl IG1haW4gcmVzb3VyY2UgaXMgbG9hZGluZywKKyAgICAgICAgaWYgdGhlcmUncyBhbnkgc3VicmVz b3VyY2UgbG9hZGluZyBvciBpZiB0aGVyZSdzIGEgcGx1Z2luCisgICAgICAgIGxvYWRpbmcuIFNv LCBpbiBjYXNlIG9mIGJlaW5nIGxvYWRpbmcsIHRob3NlIGNhc2VzIGFyZSBoYW5kbGVkCisgICAg ICAgIGluZGl2aWR1YWxseSB0byBjYW5jZWwgdGhlIG1haW4gcmVzb3VyY2UsIG9yIHNldCBhbiBl cnJvciBpbiB0aGUKKyAgICAgICAgZG9jdW1lbnQgbG9hZGVyIGFuZCBjYW5jZWwgc3VicmVzb3Vy Y2VzIGFuZCBwbHVnaW5zLCBleGNlcHQgZm9yCisgICAgICAgIHRoaXMgY2FzZSBvZiBwbHVnaW5z LCB0aGF0IG1haW5SZWNlaXZlZEVycm9yIGlzIGNhbGxlZCBpbnN0ZWFkIG9mCisgICAgICAgIHNl dHRpbmcgY2FuY2VsbGVkIGVycm9yIG9uIHRoZSBkb2N1bWVudCBsb2FkZXIuCisKKyAgICAgICAg KiBsb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RG9jdW1lbnRM b2FkZXI6OnN0b3BMb2FkaW5nKTogSWYgdGhlIGRvY3VtZW50IGlzIHN0aWxsCisgICAgICAgIGxv YWRpbmcgYmVjYXVzZSB0aGVyZSBhcmUgYWN0aXZlIHBsdWdpbnMsIHNldCB0aGUgY2FuY2VsbGVk IGVycm9yCisgICAgICAgIG9uIHRoZSBkb2N1bWVudCBpbnN0ZWFkIG9mIGNhbGxpbmcgbWFpblJl Y2VpdmVkRXJyb3IgYWdhaW4uCisKIDIwMTUtMDYtMTUgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8 Y2dhcmNpYUBpZ2FsaWEuY29tPgogCiAgICAgICAgIFtTT1VQXSBDdXN0b20gVVJJIHNjaGVtZXMg ZG9uJ3Qgd29yayBmb3IgcmVxdWVzdHMgY29udGFpbmluZyBhIGZyYWdtZW50IGlkZW50aWZpZXIK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9Eb2N1bWVudExvYWRlci5jcHAgYi9T b3VyY2UvV2ViQ29yZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwCmluZGV4IDUyZjYwZjUuLmIw YTMyNmMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9Eb2N1bWVudExvYWRlci5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50TG9hZGVyLmNwcApAQCAtMzI4 LDE0ICszMjgsMTUgQEAgdm9pZCBEb2N1bWVudExvYWRlcjo6c3RvcExvYWRpbmcoKQogICAgIGlm IChpc0xvYWRpbmdNYWluUmVzb3VyY2UoKSkgewogICAgICAgICAvLyBTdG9wIHRoZSBtYWluIHJl c291cmNlIGxvYWRlciBhbmQgbGV0IGl0IHNlbmQgdGhlIGNhbmNlbGxlZCBtZXNzYWdlLgogICAg ICAgICBjYW5jZWxNYWluUmVzb3VyY2VMb2FkKGZyYW1lTG9hZGVyLT5jYW5jZWxsZWRFcnJvciht X3JlcXVlc3QpKTsKLSAgICB9IGVsc2UgaWYgKCFtX3N1YnJlc291cmNlTG9hZGVycy5pc0VtcHR5 KCkpCi0gICAgICAgIC8vIFRoZSBtYWluIHJlc291cmNlIGxvYWRlciBhbHJlYWR5IGZpbmlzaGVk IGxvYWRpbmcuIFNldCB0aGUgY2FuY2VsbGVkIGVycm9yIG9uIHRoZSAKLSAgICAgICAgLy8gZG9j dW1lbnQgYW5kIGxldCB0aGUgc3VicmVzb3VyY2VMb2FkZXJzIHNlbmQgaW5kaXZpZHVhbCBjYW5j ZWxsZWQgbWVzc2FnZXMgYmVsb3cuCisgICAgfSBlbHNlIGlmICghbV9zdWJyZXNvdXJjZUxvYWRl cnMuaXNFbXB0eSgpIHx8ICFtX3BsdWdJblN0cmVhbUxvYWRlcnMuaXNFbXB0eSgpKSB7CisgICAg ICAgIC8vIFRoZSBtYWluIHJlc291cmNlIGxvYWRlciBhbHJlYWR5IGZpbmlzaGVkIGxvYWRpbmcu IFNldCB0aGUgY2FuY2VsbGVkIGVycm9yIG9uIHRoZQorICAgICAgICAvLyBkb2N1bWVudCBhbmQg bGV0IHRoZSBzdWJyZXNvdXJjZUxvYWRlcnMgYW5kIHBsdWdpbkxvYWRlcnMgc2VuZCBpbmRpdmlk dWFsIGNhbmNlbGxlZCBtZXNzYWdlcyBiZWxvdy4KICAgICAgICAgc2V0TWFpbkRvY3VtZW50RXJy b3IoZnJhbWVMb2FkZXItPmNhbmNlbGxlZEVycm9yKG1fcmVxdWVzdCkpOwotICAgIGVsc2UKKyAg ICB9IGVsc2UgewogICAgICAgICAvLyBJZiB0aGVyZSBhcmUgbm8gcmVzb3VyY2UgbG9hZGVycywg d2UgbmVlZCB0byBtYW51ZmFjdHVyZSBhIGNhbmNlbGxlZCBtZXNzYWdlLgogICAgICAgICAvLyAo QSBiYWNrL2ZvcndhcmQgbmF2aWdhdGlvbiBoYXMgbm8gcmVzb3VyY2UgbG9hZGVycyBiZWNhdXNl IGl0cyByZXNvdXJjZXMgYXJlIGNhY2hlZC4pCiAgICAgICAgIG1haW5SZWNlaXZlZEVycm9yKGZy YW1lTG9hZGVyLT5jYW5jZWxsZWRFcnJvcihtX3JlcXVlc3QpKTsKKyAgICB9CiAKICAgICAvLyBX ZSBhbHdheXMgbmVlZCB0byBleHBsaWNpdGx5IGNhbmNlbCB0aGUgRG9jdW1lbnQncyBwYXJzZXIg d2hlbiBzdG9wcGluZyB0aGUgbG9hZC4KICAgICAvLyBPdGhlcndpc2UgY2FuY2VsbGluZyB0aGUg cGFyc2VyIHdoaWxlIHN0YXJ0aW5nIHRoZSBuZXh0IHBhZ2UgbG9hZCBtaWdodCByZXN1bHQK