Summary: | [Cocoa] Limit set of classes that can be decoded when a preference has changed | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2020-02-20 10:00:50 PST
Created attachment 391299 [details]
Patch
Comment on attachment 391299 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391299&action=review I think this looks good, but marking r- because I think this introduces a leak. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:945 > + auto classes = [NSSet setWithArray:@[[NSString class], [NSNumber class], [NSDate class], [NSDictionary class], [NSArray class], [NSData class]]]; We should consider making this a static thing that doesn't have to get reconstructed every time there is a preference change. > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:946 > + id object = [NSKeyedUnarchiver unarchivedObjectOfClasses:classes fromData:encodedData.get() error:&err]; Don't we still need to retain the unarchived object returned by this method? Either we were over-releasing previously, or you've introduced a leak here. Created attachment 392013 [details]
Patch
(In reply to Brent Fulgham from comment #3) > Comment on attachment 391299 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391299&action=review > > I think this looks good, but marking r- because I think this introduces a > leak. > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:945 > > + auto classes = [NSSet setWithArray:@[[NSString class], [NSNumber class], [NSDate class], [NSDictionary class], [NSArray class], [NSData class]]]; > > We should consider making this a static thing that doesn't have to get > reconstructed every time there is a preference change. > Fixed. > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:946 > > + id object = [NSKeyedUnarchiver unarchivedObjectOfClasses:classes fromData:encodedData.get() error:&err]; > > Don't we still need to retain the unarchived object returned by this method? > Either we were over-releasing previously, or you've introduced a leak here. I could be wrong, but I believe this patch does not change the retain count of the object after returning from the method, since the RetainPtr variable was local. Thanks for reviewing! Created attachment 393062 [details]
Patch
Created attachment 393478 [details]
Patch
Created attachment 393491 [details]
Patch
(In reply to Brent Fulgham from comment #3) > Comment on attachment 391299 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=391299&action=review > > I think this looks good, but marking r- because I think this introduces a > leak. > > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:945 > > + auto classes = [NSSet setWithArray:@[[NSString class], [NSNumber class], [NSDate class], [NSDictionary class], [NSArray class], [NSData class]]]; > > We should consider making this a static thing that doesn't have to get > reconstructed every time there is a preference change. > For some reason, making this static introduced a crash, so I went back to declaring it as a stack allocated variable. > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:946 > > + id object = [NSKeyedUnarchiver unarchivedObjectOfClasses:classes fromData:encodedData.get() error:&err]; > > Don't we still need to retain the unarchived object returned by this method? > Either we were over-releasing previously, or you've introduced a leak here. Comment on attachment 393491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393491&action=review r=me > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:915 > + id object = [NSKeyedUnarchiver unarchivedObjectOfClasses:classes fromData:encodedData.get() error:&err]; Oh, excellent. Thank you for cleaning this one up. Comment on attachment 393491 [details]
Patch
Thanks for reviewing!
Comment on attachment 393491 [details] Patch Clearing flags on attachment: 393491 Committed r258495: <https://trac.webkit.org/changeset/258495> All reviewed patches have been landed. Closing bug. |