Bug 141026 – REGRESSION (r170576): Storage leaks in parsing of CSS image sizes

• Component: CSS
• Status: RESOLVED FIXED

[Darin Adler]

REGRESSION (r170576): Storage leaks in parsing of CSS image sizes

[Darin Adler]

Created attachment 245611 [details]
Patch

[Darin Adler]

Created attachment 245612 [details]
Patch

[Yoav Weiss]

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+)

[Anders Carlsson]

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?

[Darin Adler]

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

[Darin Adler]

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

[Darin Adler]

Created attachment 245687 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Created attachment 245772 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

I’ll upload a new patch soon. I reworked the implementation to deal with all the things I had marked FIXME.

[Darin Adler]

Created attachment 245814 [details]
Patch

[Darin Adler]

Created attachment 245857 [details]
Patch

Try to work around Windows compiler bug.

[Darin Adler]

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?

[Brent Fulgham]

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.

[WebKit Commit Bot]

Comment on attachment 245857 [details]
Patch

Clearing flags on attachment: 245857

Committed r179476: <http://trac.webkit.org/changeset/179476>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

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

[Brent Fulgham]

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>

[Brent Fulgham]

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&);

[Brent Fulgham]

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

[Brent Fulgham]

Reopening to attach new patch.

[Brent Fulgham]

Created attachment 245892 [details]
Patch

[Anders Carlsson]

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.

[Brent Fulgham]

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!

[Brent Fulgham]

Committed r179487: <http://trac.webkit.org/changeset/179487>

[Darin Adler]

Reopening to attach one ... more ... patch.

[Darin Adler]

Created attachment 245922 [details]
Patch

[Brent Fulgham]

Comment on attachment 245922 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 245922 [details]
Patch

Clearing flags on attachment: 245922

Committed r179539: <http://trac.webkit.org/changeset/179539>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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?

[Brent Fulgham]

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)

[Darin Adler]

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.

[Brent Fulgham]

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

[Csaba Osztrogonác]

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