REGRESSION (r170576): Storage leaks in parsing of CSS image sizes
Created attachment 245611 [details] Patch
Created attachment 245612 [details] Patch
Thanks for fixing! (and apologies for missing the destructor in the first place :/) I didn't know that CSSParserValue requires a "destroy()" call. Regarding the parts where the current design is not in line with the rest of CSSParser, I appreciate the feedback and would love some guidance on how this should be fixed. LGTM (I'm not sure I can officially r+)
Comment on attachment 245612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245612&action=review > Source/WebCore/css/SourceSizeList.h:81 > + m_expression = WTF::move(other.m_expression); > + m_length = other.m_length; Can you make these member initializers?
(In reply to comment #3) > Regarding the parts where the current design is not in line with the rest of > CSSParser, I appreciate the feedback and would love some guidance on how > this should be fixed. The basic approach would be to turn CSSParser data structures into CSS data structures that are not “parser” ones for persistent use. I’m not sure about all the details about exactly how to do this, but it should be relatively simple. That entire job of translation and data structure building belongs inside CSSParser. The data structures for outside the parser should not involve any CSSParser* objects.
(In reply to comment #5) > The basic approach would be to turn CSSParser data structures into CSS data > structures that are not “parser” ones for persistent use. I’m not sure about > all the details about exactly how to do this, but it should be relatively > simple. That entire job of translation and data structure building belongs > inside CSSParser. The data structures for outside the parser should not > involve any CSSParser* objects. Maybe we’d make a CSSParserSourceSize and Vector<unique_ptr<CSSParserSourceSize>> for use during parsing and then an actual SourceSizeList for use only after parsing.
Created attachment 245687 [details] Patch
Comment on attachment 245612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245612&action=review >> Source/WebCore/css/SourceSizeList.h:81 >> + m_expression = WTF::move(other.m_expression); >> + m_length = other.m_length; > > Can you make these member initializers? Yes, I did it.
Created attachment 245772 [details] Patch
Comment on attachment 245772 [details] Patch Attachment 245772 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4991503306850304 Number of test failures exceeded the failure limit.
Created attachment 245774 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 245772 [details] Patch Attachment 245772 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5123097615138816 Number of test failures exceeded the failure limit.
Created attachment 245775 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
I’ll upload a new patch soon. I reworked the implementation to deal with all the things I had marked FIXME.
Created attachment 245814 [details] Patch
Created attachment 245857 [details] Patch Try to work around Windows compiler bug.
The Windows bot still said the Windows build failed, but this time I don't see any evidence about what the error was. Maybe Brent can help me?
This looks like the EWS is in a bad state. I don't think this patch is the cause of any issue. I'll build locally and update this bug with any problems I do find.
Comment on attachment 245857 [details] Patch Clearing flags on attachment: 245857 Committed r179476: <http://trac.webkit.org/changeset/179476>
All reviewed patches have been landed. Closing bug.
I built locally and see the following error: 23> AsyncFileStream.cpp 23>c:\projects\webkit\opensource\source\webcore\css\CSSParser.cpp(1518): error C2280: 'std::unique_ptr<WebCore::MediaQueryExp,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function 23> with 23> [ 23> _Ty=WebCore::MediaQueryExp 23> ] (..\css\CSSAllInOne.cpp) 23> C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\memory(1486) : see declaration of 'std::unique_ptr<WebCore::MediaQueryExp,std::default_delete<_Ty>>::unique_ptr' 23> with 23> [ 23> _Ty=WebCore::MediaQueryExp 23> ] 23> This diagnostic occurred in the compiler generated function 'WebCore::CSSParser::SourceSize::SourceSize(const WebCore::CSSParser::SourceSize &)' 23> Blob.cpp
This might be a manifestation of this MSVC compiler bug: <http://stackoverflow.com/questions/22751203/unique-ptr-vector-trying-to-access-deleted-function-visual-studio-2013>
This looks like <https://connect.microsoft.com/VisualStudio/feedbackdetail/view/960021> The issue is that the SourceSize is being returned as a copy, which is attempting to copy the 'expression' member. I got things to build using the following change: --- css/CSSParser.cpp (revision 179478) +++ css/CSSParser.cpp (working copy) @@ -1501,6 +1501,18 @@ return result; } +CSSParser::SourceSize::SourceSize(CSSParser::SourceSize&& original) + : expression(std::move(original.expression)) + , length(original.length) +{ +} + +CSSParser::SourceSize::SourceSize(std::unique_ptr<MediaQueryExp>&& origExp, RefPtr<CSSValue> value) + : expression(std::move(origExp)) + , length(value) +{ +} + CSSParser::SourceSize CSSParser::sourceSize(std::unique_ptr<MediaQueryExp>&& expression, CSSParserValue& parserValue) { RefPtr<CSSValue> value; @@ -1511,10 +1523,9 @@ } if (!value) value = parserValue.createCSSValue(); - // FIXME: Using a named local for the result here to work around an MSVC bug. + // FIXME: Using a local for the result here to work around an MSVC bug. // With the other compilers, this works without explicitly stating the type name SourceSize or using a local. - SourceSize result { WTF::move(expression), value }; - return result; + return SourceSize(std::move(expression), value); } #endif Index: css/CSSParser.h =================================================================== --- css/CSSParser.h (revision 179478) +++ css/CSSParser.h (working copy) @@ -139,6 +139,9 @@ struct SourceSize { std::unique_ptr<MediaQueryExp> expression; RefPtr<CSSValue> length; + + SourceSize(SourceSize&&); + SourceSize(std::unique_ptr<MediaQueryExp>&& expression, PassRefPtr<CSSValue> value); }; Vector<SourceSize> parseSizesAttribute(StringView); SourceSize sourceSize(std::unique_ptr<MediaQueryExp>&&, CSSParserValue&);
(In reply to comment #23) > +CSSParser::SourceSize::SourceSize(std::unique_ptr<MediaQueryExp>&& origExp, > RefPtr<CSSValue> value) Obviously I meant PassRefPtr here in the implementation file.
Reopening to attach new patch.
Created attachment 245892 [details] Patch
Comment on attachment 245892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245892&action=review r=me if you change PassRefPtr to a RefPtr. > Source/WebCore/css/CSSParser.cpp:1504 > +CSSParser::SourceSize::SourceSize(CSSParser::SourceSize&& original) Can you use = default; here? > Source/WebCore/css/CSSParser.cpp:1510 > +CSSParser::SourceSize::SourceSize(std::unique_ptr<MediaQueryExp>&& origExp, PassRefPtr<CSSValue> value) I think this should be RefPtr<CSSValue> value. (Ideally it'd be RefPtr&& but we don't support copyRef() on RefPtr AFAIK). > Source/WebCore/css/CSSParser.cpp:1526 > + // FIXME: Using a local for the result here to work around an MSVC bug. I wouldn't call this a local. I'd say something like: // FIXME: Calling the constructor explicitly here to work around an MSVC bug. > Source/WebCore/css/CSSParser.cpp:1528 > + return SourceSize(WTF::move(expression), value); Should use WTF::move(value) here too.
Comment on attachment 245892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245892&action=review >> Source/WebCore/css/CSSParser.cpp:1504 >> +CSSParser::SourceSize::SourceSize(CSSParser::SourceSize&& original) > > Can you use = default; here? No: CSSParser.h(143): error C2610: 'WebCore::CSSParser::SourceSize::SourceSize(WebCore::CSSParser::SourceSize &&)' : is not a special member function which can be defaulted >> Source/WebCore/css/CSSParser.cpp:1510 >> +CSSParser::SourceSize::SourceSize(std::unique_ptr<MediaQueryExp>&& origExp, PassRefPtr<CSSValue> value) > > I think this should be RefPtr<CSSValue> value. (Ideally it'd be RefPtr&& but we don't support copyRef() on RefPtr AFAIK). OK! >> Source/WebCore/css/CSSParser.cpp:1526 >> + // FIXME: Using a local for the result here to work around an MSVC bug. > > I wouldn't call this a local. I'd say something like: > > // FIXME: Calling the constructor explicitly here to work around an MSVC bug. OK! >> Source/WebCore/css/CSSParser.cpp:1528 >> + return SourceSize(WTF::move(expression), value); > > Should use WTF::move(value) here too. OK!
Committed r179487: <http://trac.webkit.org/changeset/179487>
Reopening to attach one ... more ... patch.
Created attachment 245922 [details] Patch
Comment on attachment 245922 [details] Patch r=me
Comment on attachment 245922 [details] Patch Clearing flags on attachment: 245922 Committed r179539: <http://trac.webkit.org/changeset/179539>
Comment on attachment 245892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245892&action=review >>> Source/WebCore/css/CSSParser.cpp:1504 >>> +CSSParser::SourceSize::SourceSize(CSSParser::SourceSize&& original) >> >> Can you use = default; here? > > No: CSSParser.h(143): error C2610: 'WebCore::CSSParser::SourceSize::SourceSize(WebCore::CSSParser::SourceSize &&)' : is not a special member function which can be defaulted Shouldn’t this be inlined? >>> Source/WebCore/css/CSSParser.cpp:1510 >>> +CSSParser::SourceSize::SourceSize(std::unique_ptr<MediaQueryExp>&& origExp, PassRefPtr<CSSValue> value) >> >> I think this should be RefPtr<CSSValue> value. (Ideally it'd be RefPtr&& but we don't support copyRef() on RefPtr AFAIK). > > OK! Shouldn’t this be inlined?
Comment on attachment 245892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245892&action=review >>>> Source/WebCore/css/CSSParser.cpp:1504 >>>> +CSSParser::SourceSize::SourceSize(CSSParser::SourceSize&& original) >>> >>> Can you use = default; here? >> >> No: CSSParser.h(143): error C2610: 'WebCore::CSSParser::SourceSize::SourceSize(WebCore::CSSParser::SourceSize &&)' : is not a special member function which can be defaulted > > Shouldn’t this be inlined? This (and the next constructor) both require knowledge of MediaQuery, which is only forward-declared in the header. So I think it makes sense to have them inside the implementation >>>> Source/WebCore/css/CSSParser.cpp:1510 >>>> +CSSParser::SourceSize::SourceSize(std::unique_ptr<MediaQueryExp>&& origExp, PassRefPtr<CSSValue> value) >>> >>> I think this should be RefPtr<CSSValue> value. (Ideally it'd be RefPtr&& but we don't support copyRef() on RefPtr AFAIK). >> >> OK! > > Shouldn’t this be inlined? Ditto (above comment)
Comment on attachment 245892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245892&action=review >>>>> Source/WebCore/css/CSSParser.cpp:1504 >>>>> +CSSParser::SourceSize::SourceSize(CSSParser::SourceSize&& original) >>>> >>>> Can you use = default; here? >>> >>> No: CSSParser.h(143): error C2610: 'WebCore::CSSParser::SourceSize::SourceSize(WebCore::CSSParser::SourceSize &&)' : is not a special member function which can be defaulted >> >> Shouldn’t this be inlined? > > This (and the next constructor) both require knowledge of MediaQuery, which is only forward-declared in the header. So I think it makes sense to have them inside the implementation That’s unfortunate, because the compiler-generated ones would have been inlined since at all the call sites we do have “knowledge” of MediaQuery. So if we didn’t have to work around the MSVC bug we’d generate slightly better code.
(In reply to comment #37) > Comment on attachment 245892 [details] > > That’s unfortunate, because the compiler-generated ones would have been > inlined since at all the call sites we do have “knowledge” of MediaQuery. So > if we didn’t have to work around the MSVC bug we’d generate slightly better > code. All of this can go away when we update to the new Visual Studio 2015 when it is released later this year. I think I'll land a FIXME comment to remind myself to fix this code when we update.
Comment on attachment 245857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245857&action=review > Source/WebCore/html/parser/HTMLPreloadScanner.cpp:110 > - unsigned sourceSize = 0; > -#if ENABLE(PICTURE_SIZES) > - sourceSize = SourceSizeList::parseSizesAttribute(m_sizesAttribute, document.renderView(), document.frame()); > -#else > - UNUSED_PARAM(document); > -#endif > + unsigned sourceSize = parseSizesAttribute(m_sizesAttribute, document.renderView(), document.frame()); It broke !ENABLE(PICTURE_SIZES) build, because m_sizesAttribute is guarded. I filed a new bug report to fix the build: bug141327