Closed Bug 1482812 Opened Last year Closed Last year

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: tt, Assigned: tt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Based on Bug 1423917 comment 35. I'll extract patch for origin parser to here.
Priority: -- → P3
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.
Attachment #8999881 - Flags: review?(jvarga)
Attached patch fixes (obsolete) — Splinter Review
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).
Attachment #8999881 - Attachment is obsolete: true
Attachment #9001535 - Flags: review?(jvarga)
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.
Attachment #9001535 - Flags: review?(jvarga) → review+
(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
Address the comment
Attachment #9000195 - Attachment is obsolete: true
Attachment #9001535 - Attachment is obsolete: true
Attachment #9001583 - Flags: review+
Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf257868689a
Patch for turning on the origin parser check on both beta and release. r=janv
https://hg.mozilla.org/mozilla-central/rev/cf257868689a
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1485971
You need to log in before you can comment on or make changes to this bug.