Bug 145069 - [Content Extensions] Relax restrictions on triggers that match everything.
Summary: [Content Extensions] Relax restrictions on triggers that match everything.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-15 13:19 PDT by Alex Christensen
Modified: 2015-05-26 14:27 PDT (History)
3 users (show)

See Also:


Attachments
Patch (40.52 KB, patch)
2015-05-15 13:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (42.38 KB, patch)
2015-05-15 13:57 PDT, Alex Christensen
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-05-15 13:19:44 PDT
Right now we have restrictions on rules with triggers that match everything such as .*
We do not allow them after ignore-previous-rules.
We do not allow them with if-domain or unless-domain.

There are simple workarounds for these restrictions such as using [a-zA-Z]* which would have effectively the same behavior but kill performance by matching on almost every character.  We should just compile the right thing and let people be able to use .* everywhere and encourage them to put their css at the beginning.
Comment 1 Alex Christensen 2015-05-15 13:31:53 PDT
Created attachment 253219 [details]
Patch
Comment 2 Alex Christensen 2015-05-15 13:34:26 PDT
Comment on attachment 253219 [details]
Patch

I need to increment the version number and rebase this after https://bugs.webkit.org/show_bug.cgi?id=145027 but please review.
Comment 3 Alex Christensen 2015-05-15 13:57:59 PDT
Created attachment 253221 [details]
Patch
Comment 4 Alex Christensen 2015-05-15 22:53:29 PDT
Comment on attachment 253221 [details]
Patch

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

> Source/WebCore/contentextensions/DFABytecodeInterpreter.cpp:122
>                  if (instruction == DFABytecodeInstruction::AppendAction)

this should be else if
Comment 5 Benjamin Poulain 2015-05-18 14:45:28 PDT
Comment on attachment 253221 [details]
Patch

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

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:53
> +    // High bits are used to store flags. Booleans are stored in the 48th and 49th bit. See compileRuleList.

Describing the format here guarantees your comment will get out of sync.

> Source/WebCore/contentextensions/DFABytecodeCompiler.cpp:66
> +        if (action & DisplayNoneStyleSheetFlag) {
> +            RELEASE_ASSERT(!(action & IfDomainFlag));
> +            append<DFABytecodeInstruction>(m_bytecode, DFABytecodeInstruction::AppendActionDefaultStylesheet);
> +        } else if (action & IfDomainFlag)

How do you get into those cases?

not(action & 0xFFFF00000000)  is a superset of those two tests.
Comment 6 Alex Christensen 2015-05-20 10:42:19 PDT
(In reply to comment #5)
> not(action & 0xFFFF00000000)  is a superset of those two tests.
That code is correct. not(action & 0x3000000000000) is a superset of those two tests.
Comment 7 Alex Christensen 2015-05-20 10:57:18 PDT
I added a test using backend5 and fixed the else if.
http://trac.webkit.org/changeset/184644
Comment 8 Benjamin Poulain 2015-05-20 12:41:00 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > not(action & 0xFFFF00000000)  is a superset of those two tests.
> That code is correct. not(action & 0x3000000000000) is a superset of those
> two tests.

ok
Comment 9 Alex Christensen 2015-05-26 14:27:47 PDT
rdar://problem/20937620