Bug 139868 - Too large plugins are crashing (tryFastMalloc is broken with bmalloc)
Summary: Too large plugins are crashing (tryFastMalloc is broken with bmalloc)
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 142443
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-22 11:06 PST by Gabor Rapcsanyi
Modified: 2022-06-27 16:18 PDT (History)
9 users (show)

See Also:


Attachments
proposed fix (2.87 KB, patch)
2014-12-22 11:19 PST, Gabor Rapcsanyi
andersca: review-
Details | Formatted Diff | Diff
proposed fix 2 (2.98 KB, patch)
2014-12-30 09:56 PST, Gabor Rapcsanyi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabor Rapcsanyi 2014-12-22 11:06:48 PST
Too large plugins are crashing.

Test case:
<html>
  <object width="9999999999" data="test.swf"></object>
</html>
Comment 1 Gabor Rapcsanyi 2014-12-22 11:19:52 PST
Created attachment 243629 [details]
proposed fix
Comment 2 Anders Carlsson 2014-12-22 11:53:49 PST
Comment on attachment 243629 [details]
proposed fix

Patch looks good, but the test needs to use the test plug-in.
Comment 3 Gabor Rapcsanyi 2014-12-22 15:13:31 PST
(In reply to comment #2)
> Comment on attachment 243629 [details]
> proposed fix
> 
> Patch looks good, but the test needs to use the test plug-in.

Do you mean x-webkit-test-netscape? I tried it but this only happens with x-shockwave-flash. 
I can change it to 
<embed width="9999999999" src="foo.swf" type="application/x-shockwave-flash">
if you prefer that.
Comment 4 Anders Carlsson 2014-12-22 15:40:21 PST
(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 243629 [details]
> > proposed fix
> > 
> > Patch looks good, but the test needs to use the test plug-in.
> 
> Do you mean x-webkit-test-netscape? I tried it but this only happens with
> x-shockwave-flash. 
> I can change it to 
> <embed width="9999999999" src="foo.swf" type="application/x-shockwave-flash">
> if you prefer that.

My point is that we can't have tests that rely on Flash, because then we'd require people to install Flash.

Maybe we can make the test plug-in crash somehow?
Comment 5 Gabor Rapcsanyi 2014-12-30 09:56:45 PST
Created attachment 243826 [details]
proposed fix 2

This way its crashing with the test plug-in.
Comment 6 WebKit Commit Bot 2014-12-30 10:40:28 PST
Comment on attachment 243826 [details]
proposed fix 2

Clearing flags on attachment: 243826

Committed r177824: <http://trac.webkit.org/changeset/177824>
Comment 7 WebKit Commit Bot 2014-12-30 10:40:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Alexey Proskuryakov 2014-12-30 16:24:41 PST
The new test crashes on Mac with a RELEASE_ASSERT: https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK2%20(Tests)/r177825%20(1624)/plugins/large-plugin-crash-crash-log.txt

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010867bc32 bmalloc::Heap::allocateXLarge(std::__1::lock_guard<bmalloc::StaticMutex>&, unsigned long) + 98
1   com.apple.JavaScriptCore      	0x000000010867a7e7 bmalloc::Allocator::allocateXLarge(unsigned long) + 71
2   com.apple.JavaScriptCore      	0x000000010865a537 WTF::fastMalloc(unsigned long) + 151
3   com.apple.JavaScriptCore      	0x000000010865a5b1 WTF::tryFastMalloc(unsigned long) + 17

EWS did see the problem, but the patch got landed before the bubble turned red.

What's the next step here, should the patch be rolled out?
Comment 9 Alexey Proskuryakov 2014-12-31 00:40:54 PST
It's making bots way too red, and it's the only test for this fix, so skipping is not really appropriate. Rolling out.
Comment 10 Gabor Rapcsanyi 2015-02-16 04:04:05 PST
Now thats a problem on Gtk as well after enabling bmalloc.

PassRefPtr<ShareableBitmap> ShareableBitmap::create(const IntSize& size, Flags flags) 
using tryFastMalloc to allocate memory but it seems if it can't allocate enough memory it will just crash instead of returning with null.

I think we should implement the tryFastMalloc correctly or if we don't want to support this then rename it and clear the return value checks to prevent confusion.
Comment 11 Alexey Proskuryakov 2015-02-16 09:33:07 PST
So, we need to fixes to address this bug:
1. the fix to PluginProxy::updateBackingStore(),
2. and a fix to bmalloc to have tryFastMalloc actually work.
Comment 12 Radar WebKit Bug Importer 2015-02-16 09:33:23 PST
<rdar://problem/19846625>
Comment 13 Ahmad Saleem 2022-06-27 15:59:48 PDT
Attached patch was updated "PluginProxy.cpp" file, which is now removed from Webkit source (checked on Github), it was removed in following commit:

https://github.com/WebKit/WebKit/commit/043ef2367e62a2cc7e9facb1bdc42b0867b8dd6d

Is this applicable now? or this can be marked as "RESOLVED WONTFIX"? Thanks!