Bug 146019 – WebProcess crashes after too many redirect error when there's an active NPAPI plugin

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Carlos Garcia Campos]

#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 ()

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 254951 [details]
Patch

[Darin Adler]

Added Anders also.

I think this patch is right but I am not sure.

[Darin Adler]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r185643: <http://trac.webkit.org/changeset/185643>

[Alexey Proskuryakov]

> 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.

[Carlos Garcia Campos]

(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.

[Alexey Proskuryakov]

> And that's reported from ResourceHandle to the client via didFail()? 

Correct, we get a didFail().

[Carlos Garcia Campos]

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.

[Darin Adler]

Do you understand why that didn’t happen in the original case where you discovered the bug?

[Carlos Garcia Campos]

(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.

[Darin Adler]

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.