Bug 143410 – [Content Filtering] Replacement data is not always displayed when it should be

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Content Filtering] Blocked page is not always displayed when it should be

[Andy Estes]

rdar://problem/20211099

[Andy Estes]

*** Bug 142894 has been marked as a duplicate of this bug. ***

[Andy Estes]

Created attachment 250150 [details]
Patch

[WebKit Commit Bot]

Attachment 250150 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/ContentFilter.cpp:74:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/loader/ContentFilter.cpp:158:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/spi/cocoa/WebFilterEvaluatorSPI.h:35:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/WebFilterEvaluatorSPI.h:36:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/WebFilterEvaluatorSPI.h:37:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/WebFilterEvaluatorSPI.h:38:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/cocoa/ContentFilterUnblockHandlerCocoa.mm:68:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 7 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 250150 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=250150&action=review

r=me

> Source/WebCore/loader/ContentFilter.cpp:102
> +    ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData());

I would break this into two assertions so it would display the specific failure.

> Source/WebCore/loader/ContentFilter.cpp:109
> +    ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData());

Ditto.

> Source/WebCore/loader/ContentFilter.cpp:116
> +    ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData());

Ditto.

> Source/WebCore/loader/ContentFilter.cpp:173
> +    ASSERT(m_state != State::Allowed && m_state != State::Blocked);

The same idea also applies here.

> Source/WebCore/loader/ContentFilter.cpp:177
> +    // Calling m_decisionFunction might delete |this|.

Good to know!

> Source/WebCore/loader/ContentFilter.h:52
> +    ~ContentFilter() override;

One of the compilers we support used to barf on "override" on destructors. I don't recall which one. EWS will tell us I suppose.

> Source/WebCore/loader/ContentFilter.h:66
> +    RefPtr<SharedBuffer> replacementData() const;

Since this function will never return a null pointer, we should make it return Ref<SharedBuffer>.
Ditto for all of its namesake functions in this patch.

> Source/WebCore/loader/ContentFilter.h:95
> +    PlatformContentFilter* m_blockingContentFilter;
> +    State m_state;

I would initialize these here instead of doing it in the constructor's initializer list.

> Source/WebCore/loader/DocumentLoader.cpp:1585
> +            if (!unblocked && frame)

Can frame really be non-null here? I honestly have no idea.

> Source/WebCore/platform/spi/cocoa/NEFilterSourceSPI.h:71
> +

k

[Andy Estes]

(In reply to comment #5)
> Comment on attachment 250150 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250150&action=review
> 
> r=me

Thanks!

> 
> > Source/WebCore/loader/ContentFilter.cpp:102
> > +    ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData());
> 
> I would break this into two assertions so it would display the specific
> failure.

Ok.

> 
> > Source/WebCore/loader/ContentFilter.cpp:109
> > +    ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData());
> 
> Ditto.
> 
> > Source/WebCore/loader/ContentFilter.cpp:116
> > +    ASSERT(m_blockingContentFilter && m_blockingContentFilter->didBlockData());
> 
> Ditto.
> 
> > Source/WebCore/loader/ContentFilter.cpp:173
> > +    ASSERT(m_state != State::Allowed && m_state != State::Blocked);
> 
> The same idea also applies here.
> 
> > Source/WebCore/loader/ContentFilter.cpp:177
> > +    // Calling m_decisionFunction might delete |this|.
> 
> Good to know!
> 
> > Source/WebCore/loader/ContentFilter.h:52
> > +    ~ContentFilter() override;
> 
> One of the compilers we support used to barf on "override" on destructors. I
> don't recall which one. EWS will tell us I suppose.

Only clang should see this header, so I think we're okay.

> 
> > Source/WebCore/loader/ContentFilter.h:66
> > +    RefPtr<SharedBuffer> replacementData() const;
> 
> Since this function will never return a null pointer, we should make it
> return Ref<SharedBuffer>.
> Ditto for all of its namesake functions in this patch.

Ok.

> 
> > Source/WebCore/loader/ContentFilter.h:95
> > +    PlatformContentFilter* m_blockingContentFilter;
> > +    State m_state;
> 
> I would initialize these here instead of doing it in the constructor's
> initializer list.

Done.

> 
> > Source/WebCore/loader/DocumentLoader.cpp:1585
> > +            if (!unblocked && frame)
> 
> Can frame really be non-null here? I honestly have no idea.

I think it's possible, but I can move the null check earlier in this function.
> 
> > Source/WebCore/platform/spi/cocoa/NEFilterSourceSPI.h:71
> > +
> 
> k

[Andy Estes]

Committed r182356: <http://trac.webkit.org/changeset/182356>