1482812 - Turn on the build flag for validating origin in origin parser on beta and release

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement, P3)

Description

[Tom Tung [:tt, :ttung]]

Based on Bug 1423917 comment 35. I'll extract patch for origin parser to here.

Comment 1

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1482812 - Patch for turning on the origin parser check on both beta and release. r=janv

Comment 2

[Jan Varga [:janv]]

Comment on attachment 8999881 [details] [diff] [review]
Bug 1482812 - Patch for turning on the origin parser check on both beta and release. r=janv

Review of attachment 8999881 [details] [diff] [review]:
-----------------------------------------------------------------

xpcshell-parent-process.ini needs to be updated to run test_bad_origin_directory.js in all builds

I'll attach a patch that addresses some of these comments.

::: dom/quota/ActorsParent.cpp
@@ +5316,5 @@
>        return rv;
>      }
>    }
>  
> +  rv = EnsureOriginIsValid(aOrigin, directory);

I would keep the original EnsureOriginDirectory method and have only one call instead of EnsureOriginIsValid and then EnsureDirectory.
The only problem is that it must be moved to QuotaManager class, so it can access the hash table.

@@ +5442,5 @@
> +{
> +  AssertIsOnIOThread();
> +  MOZ_ASSERT(aDirectory);
> +
> +  if (auto valid = mValidOrigins.LookupForAdd(aOrigin)) {

First, we can keep sanitized origin strings in the hash table (not the original/not sanitized ones).

@@ +5444,5 @@
> +  MOZ_ASSERT(aDirectory);
> +
> +  if (auto valid = mValidOrigins.LookupForAdd(aOrigin)) {
> +    if (NS_WARN_IF(!valid.Data())) {
> +      return NS_ERROR_FAILURE;

So here, we have already tried to parse it previously and we return NS_ERROR_FAILURE, but we don't do QM_WARNING. I think we should.

There's another problem if we check the hash table very early. The original idea was to prevent creation of origin directories that can't be parsed by our origin parser. However, if we do the hash table check in the beginning of this method then it can happen we also prevent access to already created directories. I don't want to do that. It will be responsibility of next storage upgrade helper to delete such origin directories.

@@ +5493,5 @@
>      mInitializedOrigins.RemoveElement(aOrigin);
>    }
>  
> +  if (auto entry = mValidOrigins.Lookup(aOrigin)) {
> +    entry.Remove();

I don't think we want to remove it here. What if we get a request to create such origin directory right after this finishes ?
We will have to call into origin parser again.

@@ +5507,5 @@
>  {
>    AssertIsOnIOThread();
>  
>    mInitializedOrigins.Clear();
> +  mValidOrigins.Clear();

Again, the hash table is not like mInitializedOrigins which holds state information. mValidOrigins is a cache.

Comment 3

[Jan Varga [:janv]]

Attachment: fixes

Comment 4

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1482812: Patch for turning on the origin parser check on both beta and release. r=janv

Applied the fixes patch in comment 3 and addressed the comment 2 (don't remove the entries in QM::ResetOrClearCompleted() and QM::OriginClearCompleted() because it's not effective enough).

Comment 5

[Jan Varga [:janv]]

Comment on attachment 9001535 [details] [diff] [review]
Bug 1482812: Patch for turning on the origin parser check on both beta and release. r=janv

Review of attachment 9001535 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/ActorsParent.cpp
@@ +6027,5 @@
>  }
>  
> +bool
> +QuotaManager::IsSanitizedOriginValid(const nsACString& aSanitizedOrigin)
> +{

I think this we can make this even more readable/cleaner.

How about this ?
{
  AssertIsOnIOThread();
  MOZ_ASSERT(!aSanitizedOrigin.Equals(kChromeOrigin));

  bool valid;

  if (auto entry = mValidOrigins.LookupForAdd(aSanitizedOrigin)) {
    // We already parsed this sanitized origin string.
    valid = entry.Data();
  } else {
    nsCString spec;
    OriginAttributes attrs;
    OriginParser::ResultType result =
      OriginParser::ParseOrigin(aSanitizedOrigin, spec, &attrs);

    valid = result == OriginParser::ValidOrigin;
    entry.OrInsert([valid]() { return valid; });
  }

  return valid;
}

::: dom/quota/QuotaManager.h
@@ +557,5 @@
>    nsTArray<nsCString> mInitializedOrigins;
>  
> +  // A hash table records all vaild origin which were parsed successfully. This
> +  // hash table isn't protected by any mutex but it is only ever touched on the
> +  // IO thread.

- there are some typos
- we store all parsing results, not just successful ones

"A hash table that is used to cache origin parser results for given sanitized origin strings. This hash table isn't protected by any mutex but it is only ever touched on the IO thread.

Comment 6

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #5)

Thanks for the review!

> I think this we can make this even more readable/cleaner.
> 
> How about this ?
> {
>   AssertIsOnIOThread();
>   MOZ_ASSERT(!aSanitizedOrigin.Equals(kChromeOrigin));
> 
>   bool valid;
> 
>   if (auto entry = mValidOrigins.LookupForAdd(aSanitizedOrigin)) {
>     // We already parsed this sanitized origin string.
>     valid = entry.Data();
>   } else {
>     nsCString spec;
>     OriginAttributes attrs;
>     OriginParser::ResultType result =
>       OriginParser::ParseOrigin(aSanitizedOrigin, spec, &attrs);
> 
>     valid = result == OriginParser::ValidOrigin;
>     entry.OrInsert([valid]() { return valid; });
>   }
> 
>   return valid;
> }

Yeah, that makes the code much simpler and easier to read. I'll address that.
 
> ::: dom/quota/QuotaManager.h
> @@ +557,5 @@
> >    nsTArray<nsCString> mInitializedOrigins;
> >  
> > +  // A hash table records all vaild origin which were parsed successfully. This
> > +  // hash table isn't protected by any mutex but it is only ever touched on the
> > +  // IO thread.
> 
> - there are some typos
> - we store all parsing results, not just successful ones
> 
> "A hash table that is used to cache origin parser results for given
> sanitized origin strings. This hash table isn't protected by any mutex but
> it is only ever touched on the IO thread.

Will do

Comment 7

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1482812: Patch for turning on the origin parser check on both beta and release. r=janv

Address the comment

Comment 8

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18bf7f5555a5c176f9f1c509fe000dd18ad42982

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf257868689a
Patch for turning on the origin parser check on both beta and release. r=janv

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/cf257868689a