Crash due to VectorBuffer pre-allocation failure
Created attachment 426610 [details] Patch
Created attachment 426614 [details] Patch
<rdar://problem/76698411>
Comment on attachment 426614 [details] Patch I don't quite know how to review the test, but the change looks good. I think there might be a similar issue in: ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode(...) which does a `keyStatuses.reserveInitialCapacity(dataSize);` from a decoded size.
Looks like the generic HashMap and HashSet decoders also do a reservation in ArgumentCoders.h
Comment on attachment 426614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426614&action=review > Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:363 > - Vector<RefPtr<ApplePayError>> errors(size); > + Vector<RefPtr<ApplePayError>> errors; Huh, I guess this fix is correct but we should get rid of this whole encode/decode pair and just add ones for ApplePayError instead since we already have encoder/decoder for Vector<T>.
Committed r276341 (236819@main): <https://commits.webkit.org/236819@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426614 [details].
Comment on attachment 426614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426614&action=review >> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:363 >> + Vector<RefPtr<ApplePayError>> errors; > > Huh, I guess this fix is correct but we should get rid of this whole encode/decode pair and just add ones for ApplePayError instead > since we already have encoder/decoder for Vector<T>. Yes, we should do that.
(In reply to Sam Weinig from comment #5) > Looks like the generic HashMap and HashSet decoders also do a reservation in > ArgumentCoders.h Really!? That is not good. Let’s fix that. I hope there’s a way to do it without costing too much memory or performance.
(In reply to Sam Weinig from comment #4) > I think there might be a similar issue in: > > ArgumentCoder<WebCore::CDMInstanceSession::KeyStatusVector>::decode(...) > > which does a `keyStatuses.reserveInitialCapacity(dataSize);` from a decoded > size. Yes, that must be removed.
(In reply to Darin Adler from comment #9) > (In reply to Sam Weinig from comment #5) > > Looks like the generic HashMap and HashSet decoders also do a reservation in > > ArgumentCoders.h > > Really!? That is not good. Let’s fix that. I hope there’s a way to do it > without costing too much memory or performance. I don't think the reservation is too likely to be that much of a win. For avoid doing multiple allocations in the general case, we could pick a maximum value for reservations (maybe, 16 or something), and do the reservation if the size is at or below that. For memory use we could add some sort of "shrinkToFix" for HashMap/HashSet, but we would probably be better off using the new RobinHoodHashMaps if we want to improve memory use.
(In reply to Ryosuke Niwa from comment #6) > Comment on attachment 426614 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426614&action=review > > > Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:363 > > - Vector<RefPtr<ApplePayError>> errors(size); > > + Vector<RefPtr<ApplePayError>> errors; > > Huh, I guess this fix is correct but we should get rid of this whole > encode/decode pair and just add ones for ApplePayError instead > since we already have encoder/decoder for Vector<T>. Does the generic one work for T == RefPtr<Foo>?
(In reply to Sam Weinig from comment #12) > Does the generic one work for T == RefPtr<Foo>? Almost certainly yes. I meant to try it but had other code in my tree.
Did some follow-on work in bug 224984 and would like to continue to discuss there.