Closed Bug 1165214 Opened 9 years ago Closed 9 years ago

DOMStorageManager should use origin for ScopeKey and QuotaKey

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
FxOS-S5 (21Aug)
Tracking Flags:
Tracking Status
firefox41 --- wontfix
firefox46 --- fixed

People

(Reporter: allstars.chh, Assigned: mayhemer)

References

Details

Attachments

(1 file, 31 obsolete files)

vII.6
124.50 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review 
In Bug 773373 and Bug 786301 we implemented localStorage for apps on B2G, however now we will update nsIPrincial in Bug 1163254, we should try to use the new attributes of nsIPrincial as key for ScopeKey(appId:isBrowser:subdomainsDBKey) and QuotaKey(appId:isBrowser:subdomain:sheme:port).
I think that we should use .origin for the ScopeKey. But I think QuotaKey should use something like (eTLD+1)+"cookie jar".
Note that QuotaKey is already not using appId:isBrowser:subdomain:scheme:port, but rather appId:isBrowser:eTLD+1. So it should be changed to something like cookieJar:eTLD+1
(In reply to Yoshi Huang[:allstars.chh] from comment #0)
> ScopeKey(appId:isBrowser:subdomainsDBKey) and
> QuotaKey(appId:isBrowser:subdomain:sheme:port).
I mixed these two :P

Should be

ScopeKey(appId:isBrowser:subdomain:sheme:port)
QuotaDBKey(appId:isBrowser:eTLD+1)
No longer depends on: 1163254
Blocks: 1179985
Assignee: nobody → allstars.chh
Summary: DOMStorageManager should use origin as ScopeKey and QuotaKey → DOMStorageManager should use cookieJar for ScopeKey and QuotaKey
Attached patch Patch (obsolete) — — Splinter Review 
r? to Honza to he originally wrote DOMStorageManager/Observer code

feedback? to Bobby and Jonas for the usage of cookieJar attribute.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cf1b5aa79e6
Attachment #8630394 - Flags: review?(honzab.moz)
Attachment #8630394 - Flags: feedback?(jonas)
Attachment #8630394 - Flags: feedback?(bobbyholley)
Comment on attachment 8630394 [details] [diff] [review]
Patch

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

::: dom/storage/DOMStorageManager.cpp
@@ -165,5 @@
>    if (port != -1) {
>      key.Append(nsPrintfCString(":%d", port));
>    }
>  
> -  if (!aPrincipal->GetUnknownAppId()) {

I'm curious how often this fails.  But we'll see assertions in nsScriptSecurityManager.cpp!GetJarPrefix.

@@ +185,5 @@
>  
> +  aKey.Truncate();
> +  rv = AppendCookieJar(aPrincipal, aKey);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  aKey.Append(key);

changing the scope key effectively makes existing persistent database entries inaccessible.  but this is touching just any app-data, so probably not that much concern?  however, a database conversion at the start would be appreciated (see DOMStorageDBThread::InitDatabase())
Attachment #8630394 - Flags: review?(honzab.moz) → review+
Comment on attachment 8630394 [details] [diff] [review]
Patch

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

Looks good, but needs a migration as Honza mentioned.
Attachment #8630394 - Flags: feedback?(bobbyholley) → feedback+
Wait, this doesn't seem correct. Shouldn't it be using .originSuffix rather than .cookieJar. Otherwise it won't key on the whole origin?

I'm actually wondering if we should abandon the .cookieJar idea entirely. I think it adds too much complexity and in practice it is a performance optimization which is probably premature to have at this point.

Otherwise what this code (and likely a lot of other code) needs to do is to put .cookieJar in one column, and append .originSuffix-.cookieJar to the origin as a second key column.
(In reply to Jonas Sicking (:sicking) from comment #7)
> Wait, this doesn't seem correct. Shouldn't it be using .originSuffix rather
> than .cookieJar. Otherwise it won't key on the whole origin?

Gah, good catch jonas - I thought it was already using aPrincipal->GetOrigin(), but it looks like it's actually using aPrincipal->GetURI instead. So we do need originSuffix in there, or just use .origin directly - any reason not to do that? 

> I'm actually wondering if we should abandon the .cookieJar idea entirely. I
> think it adds too much complexity and in practice it is a performance
> optimization which is probably premature to have at this point.

Yeah, the extra keying of each database is annoying. But we still need a mechanism to indicate what kind of private data to clear, right?

One possibility would be to just send a JSON-ified object "clear-cookiejar-data" notification of the form { attr1: value1, attr2: value2 }. The implementation would then rehydrate the OriginAttributes for each entry in the database, and clear any entry that matches each of the parameters sent.
Thanks for the comments.

So I'll do
1. change AppendCookieJar to AppendOriginSuffix.
2. notify OriginAttributeDictionary in the subject parameter.
3. When we receive clear-cookiejar-data, I'll use ChromeUtils::OriginAttributesToSuffix to get the originSuffix, then clean the private data.

Is this okay?
Summary: DOMStorageManager should use cookieJar for ScopeKey and QuotaKey → DOMStorageManager should use origin for ScopeKey and QuotaKey

        Attachment #8630394 -
        Attachment is obsolete: true

        Attachment #8630394 -
        Flags: feedback?(jonas)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #5)
> @@ +185,5 @@
> >  
> > +  aKey.Truncate();
> > +  rv = AppendCookieJar(aPrincipal, aKey);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  aKey.Append(key);
> 
> changing the scope key effectively makes existing persistent database
> entries inaccessible.  but this is touching just any app-data, so probably
> not that much concern?  however, a database conversion at the start would be
> appreciated (see DOMStorageDBThread::InitDatabase())

Hi Honza
Thanks for the review.
However I am not sure about the conversion part here.

My idea is 
1. Check if webappsstore2 exists or not.
If it exists,
2. rename webappsstore2 to webappsstore2_old
3. Convert the old scope key (appId:isInBrowser:) to originSuffix (through OriginAttributes and ChromeUtils classes)
4. import the converted data to a new table webappsstore2.
5. delete webappsstore2_old table.

Am I correct about this?
Or if not, can you help to point out where I did it wrong ?

Thanks

    
    

    
  
Comment on attachment 8631515 [details] [diff] [review]
Part 1: notify originAttributes in clear-cookiejar-data

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

Hi, Bobby
Is this what you meant in Comment 8?

Thanks

        Attachment #8631515 -
        Flags: review?(bobbyholley)

    
    

    
  
Comment on attachment 8631517 [details] [diff] [review]
Part 2: use origin in DOMStorageManager.

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

change to use originSuffix.

Once Honza provides some suggestions for the database conversion, I'll upload another part and r? to him.

Thanks

        Attachment #8631517 -
        Flags: review?(bobbyholley)

        Attachment #8631517 -
        Flags: feedback?(jonas)

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #13)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > @@ +185,5 @@
> > >  
> > > +  aKey.Truncate();
> > > +  rv = AppendCookieJar(aPrincipal, aKey);
> > > +  NS_ENSURE_SUCCESS(rv, rv);
> > > +  aKey.Append(key);
> > 
> > changing the scope key effectively makes existing persistent database
> > entries inaccessible.  but this is touching just any app-data, so probably
> > not that much concern?  however, a database conversion at the start would be
> > appreciated (see DOMStorageDBThread::InitDatabase())
> 
> Hi Honza
> Thanks for the review.
> However I am not sure about the conversion part here.
> 
> My idea is 
> 1. Check if webappsstore2 exists or not.
> If it exists,
> 2. rename webappsstore2 to webappsstore2_old
> 3. Convert the old scope key (appId:isInBrowser:) to originSuffix (through
> OriginAttributes and ChromeUtils classes)
> 4. import the converted data to a new table webappsstore2.
> 5. delete webappsstore2_old table.
> 
> Am I correct about this?
> Or if not, can you help to point out where I did it wrong ?
> 
> Thanks

And how will you do it on next start?  And the next one?  Problem is we've never introduced usage of the schema version numbering for the localstorage database.  We always copy to new tables when we find old ones.  So, there are two ways:

- introduce new webappsstore3 table and drop webappsstore2 after the copy/conversion is done
- introduce schema versioning (preferred) ; that will for most users with only web data be back and forth compatible and we will (finally!) have the proper schema versioning
Depends on: 1182347

    
    

    
  
See bug 1182347, which may alter the story here a little bit.

        Attachment #8631517 -
        Flags: review?(bobbyholley)

        Attachment #8631517 -
        Flags: feedback?(jonas)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: DB conversion. (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8631517 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8639697 [details] [diff] [review]
Part 1: use OriginSuffix in DOMStorageManager

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

Hi, Bobby and Jonas
Sorry for not response for a while, I was busy on my private stuff.
I've changed it to use originSuffix, 
can you review this again?

Thanks

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

        Attachment #8639697 -
        Flags: review?(jonas)

        Attachment #8639697 -
        Flags: review?(bobbyholley)

    
    

    
  
Comment on attachment 8639697 [details] [diff] [review]
Part 1: use OriginSuffix in DOMStorageManager

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

This looked good at a glance, but Bobby should review

        Attachment #8639697 -
        Flags: review?(jonas)

    
    

    
  
Comment on attachment 8639698 [details] [diff] [review]
Part 2: DB conversion.

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

Hi smaug, 
I was planning to send r? to :mayhemer, but seems he's busy.
So I flag r? to you as I found out you are the original reviewer for the code here.

Please see comment 16 for what mayhemer suggested.
Sorry the patch looks a mess,
but what I only did is to add a switch case and add the conversion check in line 563.

Thanks

        Attachment #8639698 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8639698 [details] [diff] [review]
Part 2: DB conversion.

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

please provide a version of the patch ignoring writespace changes (use -w option on qdiff/diff)

::: dom/storage/DOMStorageDBThread.cpp
@@ +562,3 @@
>  
> +        if (NS_IsAsciiDigit(foundScope.First())) {
> +          int index = foundScope.FindChar(':');

please use mozilla::Tokenizer (mozilla/Tokenizer.h) for this.

OTOH I don't understand why you need to convert here, can you explain?  the scope column should (after the database conversion above) be what the CreateScopeKey() (carrying further to DOMStorageCache::Scope()) returns, which is the value this is compared/mapping to.

This is part of a very important anti-main thread blocking optimization and must not be broken!

@@ +575,5 @@
> +        MonitorAutoLock monitor(mThreadObserver->GetMonitor());
> +        mScopesHavingData.PutEntry(foundScope);
> +      }
> +
> +      rv = mWorkerConnection->SetSchemaVersion(1);

do this as part of the update transaction

    
    

    
  
Comment on attachment 8639698 [details] [diff] [review]
Part 2: DB conversion.

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

::: dom/storage/DOMStorageDBThread.cpp
@@ +541,3 @@
>  
> +      rv = transaction.Commit();
> +      NS_ENSURE_SUCCESS(rv, rv);

and btw, you must commit the transaction at the same block level as you open it (i.e. - out of the switch apparently)

    
    

    
  
Comment on attachment 8639698 [details] [diff] [review]
Part 2: DB conversion.

Looks like mayhemer might be reviewing this one after all.
And anyhow, he provided some feedback, so I assume the patch will be updated.

        Attachment #8639698 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8639697 [details] [diff] [review]
Part 1: use OriginSuffix in DOMStorageManager

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

r=me with those things addressed, but I think that Jan (or another person who knows this code) should look at this too.

::: dom/base/ChromeUtils.cpp
@@ +27,2 @@
>    OriginAttributes attrs(aAttrs);
>    attrs.CreateSuffix(aSuffix);

I don't think we should call into ChromeUtils from DOMStorageObserver. Please revert this change and just call attrs.CreateSuffix() directly from other C++ code.

::: dom/storage/DOMStorageManager.cpp
@@ +130,5 @@
> +  nsAutoCString suffix;
> +  nsresult rv = aPrincipal->GetOriginSuffix(suffix);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (suffix.IsEmpty()) {

I think it we should append the same number of ':' separators regardless of whether the suffix is empty. It makes parsing a lot easier and less spoofable.

So I think we should remove the .IsEmpty() check here.

@@ +135,5 @@
> +    return NS_OK;
> +  }
> +
> +  aKey.Append(suffix);
> +  aKey.Append(':');

I think appending the ':' should happen in the caller.

::: dom/storage/DOMStorageObserver.cpp
@@ +273,5 @@
>  
>      DOMStorageDBBridge* db = DOMStorageCache::StartDatabase();
>      NS_ENSURE_TRUE(db, NS_ERROR_FAILURE);
>  
> +    db->AsyncClearMatchingScope(suffix);

This is going to do the wrong thing when |suffix| is empty, right? It seems like you need to append the ':' on both sides here.

        Attachment #8639697 -
        Flags: review?(bobbyholley)

        Attachment #8639697 -
        Flags: review?(Jan.Varga)

        Attachment #8639697 -
        Flags: review+

    
    

    
  
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #23)
> ::: dom/storage/DOMStorageDBThread.cpp
> @@ +562,3 @@
> >  
> > +        if (NS_IsAsciiDigit(foundScope.First())) {
> > +          int index = foundScope.FindChar(':');
> 
> please use mozilla::Tokenizer (mozilla/Tokenizer.h) for this.
> 
> OTOH I don't understand why you need to convert here, can you explain?  the
> scope column should (after the database conversion above) be what the
> CreateScopeKey() (carrying further to DOMStorageCache::Scope()) returns,
> which is the value this is compared/mapping to.
> 
> This is part of a very important anti-main thread blocking optimization and
> must not be broken!

Hi Honza
Sorry I may misunderstand what you meant in Comment 5.
I thought "a database conversion at the start would be appreciated" is to convert the old entries in "scope" column (appId:isInBrowserElement:) to OriginSuffix.

Could you be more specific about the database conversion here?

Thanks

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #27)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #23)
> > ::: dom/storage/DOMStorageDBThread.cpp
> > @@ +562,3 @@
> > >  
> > > +        if (NS_IsAsciiDigit(foundScope.First())) {
> > > +          int index = foundScope.FindChar(':');
> > 
> > please use mozilla::Tokenizer (mozilla/Tokenizer.h) for this.
> > 
> > OTOH I don't understand why you need to convert here, can you explain?  the
> > scope column should (after the database conversion above) be what the
> > CreateScopeKey() (carrying further to DOMStorageCache::Scope()) returns,
> > which is the value this is compared/mapping to.
> > 
> > This is part of a very important anti-main thread blocking optimization and
> > must not be broken!
> 
> Hi Honza
> Sorry I may misunderstand what you meant in Comment 5.
> I thought "a database conversion at the start would be appreciated" is to
> convert the old entries in "scope" column (appId:isInBrowserElement:) to
> OriginSuffix.
> 
> Could you be more specific about the database conversion here?
> 
> Thanks

From a quick look the database conversion seems right.

But I don't understand why are you filling the mScopesHavingData in-memory hashtable with something else than you've read from the database (why you convert those values).

    
    

    
  
aha, so I think I should do the conversion in the beginning.

switch (schemaVer) {
  case 0: {
    // check if webappsstore2 exist.
    if (exists) {
      // do the appId:isInBrowser -> OriginSuffix convertsion.
    } else {
      // CREATE TABLE webappsstore2
      // CREATE UNIQUE INDEX IF NOT EXISTS scope_key_index ON webappsstore2(scope, key)
    }

    ....  
  }

}


Am I correct?
Sorry for asking this back and forth.
I'll ping you on irc later. :D

Thanks

    
    

    
  


  
addressed bholley's comments.

        Attachment #8639697 -
        Attachment is obsolete: true

        Attachment #8639697 -
        Flags: review?(Jan.Varga)

        Attachment #8641627 -
        Flags: review?(Jan.Varga)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: DB conversion. v2. (obsolete)
        — 
          — Splinter Review
      

    


  
Hi Honza
Could you help to check this again?

I'll upload another patch with -w.

Thanks

        Attachment #8639698 -
        Attachment is obsolete: true

    
  
Flags: needinfo?(honzab.moz)

    
    

    
  
Comment on attachment 8641631 [details] [diff] [review]
Part 2: DB conversion. v2. , with -w option

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

thanks for the -w patch

::: dom/storage/DOMStorageDBThread.cpp
@@ +464,5 @@
> +        break;
> +      default:
> +        break;
> +    }
> +  }

yes, but better would be to do it a slightly different way.

according [1] the format is:

[appid:inbrowser:]reversed-domain.:schema:port

so, you need to do the following list of sequential steps:

- check for an integer, if it's there, it's your app id
- check for ':'
- check for one of "t" or "f" words
- check for ':'
- capture the rest of the string (the "reversed-domain.:schema:port" part that needs to be appended)

if any of this fails, just return |scope| unchanged.

also, you are forgetting to append the part after the second colon...  are you testing your code?  the result must be in parity with what you are coding in CreateScopeKey


Also, I'll soon land some improvements to the Tokenizer, see bug 1188983.  You may use them here (mainly the ReadInteger, ReadWord methods)



[1] http://hg.mozilla.org/mozilla-central/annotate/33dc8a83cfc0/dom/storage/DOMStorageManager.cpp#l179

        Attachment #8641631 -
        Flags: review-

    
    

    
  
Comment on attachment 8641627 [details] [diff] [review]
Part 1: use OriginSuffix in DOMStorageManager v2.

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

::: dom/storage/DOMStorageManager.cpp
@@ +180,5 @@
>      key.Append(nsPrintfCString(":%d", port));
>    }
>  
> +  if (aPrincipal->GetUnknownAppId()) {
> +    return NS_OK;

nit - we should do aKey.Assign(key) here - it seems to be a long standing bug..

    
    

    
  
Feedback given.
Flags: needinfo?(honzab.moz)

    
    

    
  
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #34)
> >  
> > +  if (aPrincipal->GetUnknownAppId()) {
> > +    return NS_OK;
> 
> nit - we should do aKey.Assign(key) here - it seems to be a long standing
> bug..

Thanks for the reminder, 
In Comment 26 Bobby also suggested we should use ':' even when the suffix is empty.
So I'll update part 1 and part 2 accordingly.

    
    

    
  
Comment on attachment 8641627 [details] [diff] [review]
Part 1: use OriginSuffix in DOMStorageManager v2.

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

::: dom/storage/DOMStorageObserver.cpp
@@ +268,5 @@
>      }
>  
> +    nsAutoCString suffix;
> +    attrs.CreateSuffix(suffix);
> +    suffix.Append(':');

This line should be removed,
per Bobby's comment from https://bugzilla.mozilla.org/show_bug.cgi?id=1182347#c0

<quote>
each attribute makes the operation more restrictive. Sending {appId: 42} clears all data associated with app 42, whereas sending {appId: 42, inBrowser: true} clears all data that is associated with app 42 _and also_ is in a mozBrowser.
</quote>

So when an app is uninstalled, not only the app's data jar but also the browserElement's data jar of the app should be removed.

    
    

    
  


  

        Attachment #8641627 -
        Attachment is obsolete: true

        Attachment #8641627 -
        Flags: review?(Jan.Varga)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 2: DB conversion. v3. (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8641630 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8642981 [details] [diff] [review]
Part 1: use OriginSuffix in DOMStorageManager v3.

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

Seems Jan is busy, and since Honza has already checked my patch and also reviewed Part 2 before,
I'll change the r? to Honza.

Also I've pointed out the change since v2 below.

Honza, could you help to review this for me?

Thanks

::: dom/storage/DOMStorageManager.cpp
@@ +187,3 @@
>    }
>  
> +  aKey.Append(':');

As requested by Bobby in Comment 26, always append a : here.

::: dom/storage/DOMStorageObserver.cpp
@@ +219,3 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    scope.Append(":");
> +    scope.Append(domain);

Since the scope key will have ':', so for the "perm-changed" notification I also append a ':' here.

@@ +248,5 @@
>      DOMStorageDBBridge* db = DOMStorageCache::StartDatabase();
>      NS_ENSURE_TRUE(db, NS_ERROR_FAILURE);
>  
> +    scopePrefix.Append(":");
> +    scopePrefix.Append(domain);

Same for "browser:purge-domain-data" notification.

    
    

    
  
Comment on attachment 8642985 [details] [diff] [review]
Part 2: DB conversion. v3. , with -w option

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

Hi Honza
I've addressed your comment 33.
Also seems bug 1188983 is not reviewed yet, so I didn't apply your latest change in Tokenizer yet.
Once your patch is r+, I'll rebase on it.

Thanks

    
    

    
  
ni? Honza for reviewing patches in Comment 41 and Comment 42.
Flags: needinfo?(honzab.moz)
Target Milestone: --- → FxOS-S5 (21Aug)

    
    

    
  
Comment on attachment 8642985 [details] [diff] [review]
Part 2: DB conversion. v3. , with -w option

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

::: dom/storage/DOMStorageDBThread.cpp
@@ +33,5 @@
>  
>  // Write Ahead Log's maximum size is 512KB
>  #define MAX_WAL_SIZE_BYTES 512 * 1024
>  
> +#define DB_SCHEMA_VERSION 1

please don't have a define for this, it's useless.

@@ +452,5 @@
> +    // bail out if it isn't appId.
> +    if (!tokenizer.Check(Tokenizer::TOKEN_INTEGER, t)) {
> +      break;
> +    }
> +    int appId = t.AsInteger();

uint32_t please

@@ +464,5 @@
> +    if (!tokenizer.Check(Tokenizer::TOKEN_WORD, t)) {
> +      break;
> +    }
> +
> +    bool inBrowser = t.AsString() == "t";

must be one of "t" or "f".  If it's something else, bail

@@ +579,5 @@
> +                             "SELECT TOSUFFIX(scope), key, value, secure, owner "
> +                             "FROM webappsstore2"));
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +      } else {

this is also done absolutely wrong... you must first finish all conversion from old profiles and only after that you must do the origin conversion.  this must be the last operation you do in the v0 case

@@ +662,5 @@
>          rv = stmt->GetUTF8String(0, foundScope);
>          NS_ENSURE_SUCCESS(rv, rv);
>  
>          MonitorAutoLock monitor(mThreadObserver->GetMonitor());
>          mScopesHavingData.PutEntry(foundScope);

this is just wrong, don't you see that?

this must be done outside the conversion transaction and in all cases, not just the v0

        Attachment #8642985 -
        Flags: review-

    
  
Flags: needinfo?(honzab.moz)

    
    

    
  
Comment on attachment 8642985 [details] [diff] [review]
Part 2: DB conversion. v3. , with -w option

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

Hi Honza
Thanks for your review.

Sorry I am a SQL-dummy so didn't know the secrets on the SQL stuff.
And you didn't point out the problem in the last review so I thought it's okay :P.
Thanks for pointing this out.

I didn't find the tests for the updating the webappsstore2 so I only tested the nsToSuffixSQLFunction but not the SQL statements.
Is there some SQL test I could leverage?
Or is there some web page could test dom storage so I could manually verifying the SQL statement conversion?

::: dom/storage/DOMStorageDBThread.cpp
@@ +33,5 @@
>  
>  // Write Ahead Log's maximum size is 512KB
>  #define MAX_WAL_SIZE_BYTES 512 * 1024
>  
> +#define DB_SCHEMA_VERSION 1

Did you mean using const int32_t for it?
if not, can you be more specific?

    
    

    
  
If you don't mind I'll rather finish the patch.  It's easier to update the patch than explain what all needs to be done.
Assignee: allstars.chh → honzab.moz
Status: NEW → ASSIGNED

    
    

    
  


  
I'll submit a -w patch too.

- fixes the ordering of statements w/o disruption of the existing code
- fixed the update statement to actually work
- manually tested for existing appid:inbrowser format
- tested the data are preserved after the update
- tested the preload feature is not broken

        Attachment #8642982 -
        Attachment is obsolete: true

        Attachment #8642985 -
        Attachment is obsolete: true

        Attachment #8644913 -
        Flags: review?(bugs)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v4 -w (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
Comment on attachment 8644913 [details] [diff] [review]
Part 2: DB conversion. v4 (by Honza Bambas)

Sorry, I have no idea what ScopeUpdateToOriginSuffixSQLFunction is supposed to do.
I think bholley should review.

        Attachment #8644913 -
        Flags: review?(bugs) → review?(bobbyholley)

    
    

    
  
Comment on attachment 8644913 [details] [diff] [review]
Part 2: DB conversion. v4 (by Honza Bambas)

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

The OriginSuffix stuff looks good, but I'm not familiar enough with the database pieces to review them quickly. Regardless, I'm swamped with stuff and don't want to sit on this.

If self-review is good enough here, that's totally fine by me.

::: dom/storage/DOMStorageDBThread.cpp
@@ +495,5 @@
> +    tokenizer.Claim(key);
> +    newScope.Append(key);
> +  } else {
> +    // return the original scope if failure.
> +    newScope.Append(oldScope);

Don't we need to return failure in this case? Otherwise we'll think that the scheme has been updated when it hasn't.

        Attachment #8644913 -
        Flags: review?(bobbyholley) → feedback+

    
    

    
  
(In reply to Bobby Holley (Mostly Out This Week) from comment #50)
> Comment on attachment 8644913 [details] [diff] [review]
> Part 2: DB conversion. v4 (by Honza Bambas)
> 
> Review of attachment 8644913 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The OriginSuffix stuff looks good, but I'm not familiar enough with the
> database pieces to review them quickly. Regardless, I'm swamped with stuff
> and don't want to sit on this.
> 
> If self-review is good enough here, that's totally fine by me.
> 
> ::: dom/storage/DOMStorageDBThread.cpp
> @@ +495,5 @@
> > +    tokenizer.Claim(key);
> > +    newScope.Append(key);
> > +  } else {
> > +    // return the original scope if failure.
> > +    newScope.Append(oldScope);
> 
> Don't we need to return failure in this case? Otherwise we'll think that the
> scheme has been updated when it hasn't.

The "if failure" is just a bad wording.  It means we haven't found the "appid:inbrowser:" string in the existing key.  The var name should be changed to "found" and the "if failure" should be "if not found".  I'll rework it even more.

Thanks.
Flags: needinfo?(honzab.moz)

    
    

    
  
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #51)
> The "if failure" is just a bad wording.  It means we haven't found the
> "appid:inbrowser:" string in the existing key.  The var name should be
> changed to "found" and the "if failure" should be "if not found".  I'll
> rework it even more.

But it seems like rather than bailing out in that case, we still want to add the ':' unconditionally, right?

    
    

    
  
(In reply to Bobby Holley (Mostly Out This Week) from comment #52)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #51)
> > The "if failure" is just a bad wording.  It means we haven't found the
> > "appid:inbrowser:" string in the existing key.  The var name should be
> > changed to "found" and the "if failure" should be "if not found".  I'll
> > rework it even more.
> 
> But it seems like rather than bailing out in that case, we still want to add
> the ':' unconditionally, right?

Sure.

    
    

    
  


  

        Attachment #8644913 -
        Attachment is obsolete: true

        Attachment #8644914 -
        Attachment is obsolete: true
Flags: needinfo?(honzab.moz)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v5 -w (obsolete)
        — 
          — Splinter Review
      

    


  
Olly, I have to bother you with one more localStorage patch, can you take a look please?

Depends on bug 1188983 which is close to land.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4324181f6c5e

        Attachment #8647652 -
        Flags: review?(bugs)

    
  
Depends on: 1188983

    
    

    
  
Comment on attachment 8647652 [details] [diff] [review]
v5 -w

>+class ScopeUpdateToOriginSuffixSQLFunction final : public mozIStorageFunction
>+{
>+  ~ScopeUpdateToOriginSuffixSQLFunction() {}
>+
>+  static bool
>+  ConvertAppIdAndInBroser(const nsACString &oldScope, nsACString &newScope);
nit
const nsACString& aOldScope, nsACString& aNewScope

>+ScopeUpdateToOriginSuffixSQLFunction::ConvertAppIdAndInBroser(
>+  const nsACString &oldScope, nsACString &newScope)
ditto

I assume bholley reviewed this method.

>+
>+    rv = mWorkerConnection->SetSchemaVersion(1);
this

>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // No break
>+  }
>+  case 1: {
and this really could use some defined CURRENT_SCHEMA_VERSION

        Attachment #8647652 -
        Flags: review?(bugs) → review+

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #56)
> >+
> >+    rv = mWorkerConnection->SetSchemaVersion(1);
> this
> really could use some defined CURRENT_SCHEMA_VERSION

I don't agree with this place.  Logically, when we add a new version and a new update code, we still want 1 here.  But CURRENT_SCHEMA_VERSION will already be changed to 2.  Hence this SetSchemaVersion would actually be wrong.

    
    

    
  
allstars: will you ask for review of the "Part 1" patch?  The patches are stalling here, we should move forward...  Thanks.
Flags: needinfo?(allstars.chh)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1/2: DB conversion, v5.1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8647646 -
        Attachment is obsolete: true

        Attachment #8647652 -
        Attachment is obsolete: true

        Attachment #8661252 -
        Flags: review+

    
    

    
  
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #58)
> allstars: will you ask for review of the "Part 1" patch?  The patches are
> stalling here, we should move forward...  Thanks.

I have asked for your review in Comment 41.
But if you cannot, I'll send r? to Jan.
Flags: needinfo?(allstars.chh)

    
  

        Attachment #8642981 -
        Flags: review?(Jan.Varga) → review?(honzab.moz)

    
    

    
  
I ask Jan, he is OK me to take the review.

    
    

    
  
Yoshi, you had to ni? me in comment 41.  We would save this delay.

    
    

    
  
Yeah, I ni? you in comment 43. I guess you didn't notice that :P

    
    

    
  
Ah, I missed the first patch request, sorry :)

    
    

    
  
Comment on attachment 8642981 [details] [diff] [review]
Part 1: use OriginSuffix in DOMStorageManager v3.

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

r=honzab with all the comments fixed.

::: dom/storage/DOMStorageObserver.cpp
@@ +214,5 @@
>        return NS_OK;
>      }
>  
> +    nsAutoCString scope, domain;
> +    rv = CreateReversedDomain(host, domain);

the 'domain' variable should be renamed to 'reversedDomain'

@@ +219,2 @@
>      NS_ENSURE_SUCCESS(rv, rv);
> +    scope.Append(":");

blank line before this line

@@ +241,5 @@
>                     aceDomain);
>      }
>  
> +    nsAutoCString scopePrefix, domain;
> +    rv = CreateReversedDomain(aceDomain, domain);

same name here.

@@ +266,5 @@
>  
> +  // Clear data of the origins whose prefixes will match the suffix.
> +  if (!strcmp(aTopic, "clear-origin-data")) {
> +    OriginAttributes attrs;
> +    if (!attrs.Init(nsString(aData))) {

s/nsString/nsDepenendentString/

@@ +276,5 @@
>  
>      DOMStorageDBBridge* db = DOMStorageCache::StartDatabase();
>      NS_ENSURE_TRUE(db, NS_ERROR_FAILURE);
>  
> +    db->AsyncClearMatchingScope(suffix);

You must yet append ':' to the suffix!  The statement parameter just appends % for use with LIKE and there can be an overlap.

        Attachment #8642981 -
        Flags: review?(honzab.moz) → review+

    
    

    
  
Comment on attachment 8642981 [details] [diff] [review]
Part 1: use OriginSuffix in DOMStorageManager v3.

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

::: dom/storage/DOMStorageObserver.cpp
@@ +276,5 @@
>  
>      DOMStorageDBBridge* db = DOMStorageCache::StartDatabase();
>      NS_ENSURE_TRUE(db, NS_ERROR_FAILURE);
>  
> +    db->AsyncClearMatchingScope(suffix);

Actually we cannot append ':' here.
When an app is uninstalled, we need to remove its data jar , and its data jar for the browser element,

for example, when an app with appId=1001 is uninstalled,
clear-origin-data will be notified with data is "^appId=1001",

we need to clear two entries in the DOMStorageDBThread
1. ^appId=1001:moc.tset.:http:80  (with reversed domain of http://test.com:80)
2. ^appId=1001&inBrowser=1:moc.tset.:http:80

So appending ':' will make it not match the 2nd entry.

See Bobby's patch in Bug 1182347.

    
    

    
  
ni? Honza to confirm comment 67 first.
Flags: needinfo?(honzab.moz)

    
    

    
  


  
Addressed Honza's comments.

        Attachment #8642981 -
        Attachment is obsolete: true

        Attachment #8664625 -
        Flags: review+

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #67)
> Comment on attachment 8642981 [details] [diff] [review]
> Part 1: use OriginSuffix in DOMStorageManager v3.
> 
> Review of attachment 8642981 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/storage/DOMStorageObserver.cpp
> @@ +276,5 @@
> >  
> >      DOMStorageDBBridge* db = DOMStorageCache::StartDatabase();
> >      NS_ENSURE_TRUE(db, NS_ERROR_FAILURE);
> >  
> > +    db->AsyncClearMatchingScope(suffix);
> 
> Actually we cannot append ':' here.
> When an app is uninstalled, we need to remove its data jar , and its data
> jar for the browser element,
> 
> for example, when an app with appId=1001 is uninstalled,
> clear-origin-data will be notified with data is "^appId=1001",
> 
> we need to clear two entries in the DOMStorageDBThread
> 1. ^appId=1001:moc.tset.:http:80  (with reversed domain of
> http://test.com:80)
> 2. ^appId=1001&inBrowser=1:moc.tset.:http:80
> 
> So appending ':' will make it not match the 2nd entry.
> 
> See Bobby's patch in Bug 1182347.

No.  You are wrong.  The colon must be there.

The clear-origin-data notification, as I understand it, tells you to delete data exactly from that origin.  We no longer want to do any automatic inbrowser=* deletions!  That is the purpose of bug 1168777.

The layer invoking the clear-origin-data notification is responsible to call it for all combinations (origins) needed.  It's not your job to do here.

Regardless that, however moot it is now, other problem is that your approach counts with (non-deterministic at all !!!) order of the key=value pairs in the suffix string.  You count with the fact that appid=N is the first one.  That is definitely a very bad thing to do!  You can no longer do any trivial appid=nnnn% sqlite tricks.  But you are not going to do them anyway, so no concern, brought in just for completeness.
Flags: needinfo?(honzab.moz)

    
    

    
  


  
Appending ':'.

        Attachment #8664625 -
        Attachment is obsolete: true

    
    

    
  
What is this bug blocked on?
Flags: needinfo?(allstars.chh)

    
    

    
  
Sorry I have no idea.
I thought you took the assignee and will work on this bug.
Flags: needinfo?(allstars.chh)

    
    

    
  
Oh, you are right, sorry!  I was confused why there was r+ missing on your last patch.  As I understand it can be r+'ed and landed, right?
Flags: needinfo?(allstars.chh)

    
    

    
  
The part 1 is already r+'ed by you in Comment 66, and also append the ':' with your request in Comment 71, sorry I forgot to carry over the r+ at that time.
But appending the ':' will cause some failures in try as I mentioned in Comment 67.

It's okay to me whether you'd like to reuse my part 1 patch or not since you took this bug.
Whether you want to rewrite it is also fine by me, since you know this code more than me :D.

Also to avoid we two work on the same bug, I didn't work on this bug anymore since the day you took this bug.
I finish Part 1 just because I've sent r? to you before you took this bug.

To my understanding, since you took it, then it's your resposibility now.

If you'd like me to finish and land Part 1, then I'd like to take this bug back and start working on it, maybe I will seperate your Part 2 patch to another bug to avoid confusion.

Thanks
Flags: needinfo?(allstars.chh)

    
    

    
  
Thanks for this explanation.  I'll take care of this bug then.

    
  

        Attachment #8661252 -
        Attachment description: v5.1 → 2/2: DB conversion, v5.1

    
    

    
  


  
- general polishing
- "session-only-cleared" added the origin-suffix prefix

The code seems to me to be correct, tests will show.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b808849554a

        Attachment #8666433 -
        Attachment is obsolete: true

        Attachment #8672692 -
        Flags: review?(allstars.chh)

    
    

    
  
The only failing test:

dom/tests/mochitest/localstorage/test_app_uninstall.html | localstorage data have been deleted - got "bar", expected null

    
    

    
  
The problem is that "clear-origin-data" is called only for the 'inBrowser=false' variant, tho there are data for inBrowser=true as well.  The test expects those also be deleted.

So, here I either don't follow how "clear-origin-data" should work or how should it be handled.  I was hoping that consumers of the notification no longer bear the obligation to delete also inBrowser=true data.

    
    

    
  
Shouldn't clear-origin-data be using OriginAttributesPattern?

    
    

    
  
Comment on attachment 8672692 [details] [diff] [review]
1/2: DOMStorageManager use OriginAttributes, v6

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

Sorry but I don't think I am qualified enough to review this. :(

        Attachment #8672692 -
        Flags: review?(allstars.chh)

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #80)
> Shouldn't clear-origin-data be using OriginAttributesPattern?

Bobby, are there some instructions I can read how this stuff is supposed to be exactly used?  There is nothing useful in the code around the OriginAttributes* declarations.

Thanks.
Flags: needinfo?(bobbyholley)

    
    

    
  
// aJSONString is the aData from clear-origin-data, i.e. the 3rd argument of the notification
OriginAttributesPattern pattern;
if (!pattern.Init(aJSONString)) {
  // handle failure.
}

then match the pattern by
ChromeUtils.originAttributesMatchPattern(stored_origin_attributes, pattern)

    
    

    
  
It's really great to land APIs w/o any documentation...  Can you please tell me what each of the arguments to OriginAttributesMatchPattern means and how this should exactly be used?

    
  

        Attachment #8672692 -
        Attachment is obsolete: true

    
  

        Attachment #8661252 -
        Attachment description: 2/2: DB conversion, v5.1 → 1/2: DB conversion, v5.1

    
    

    
  
I love OriginAttributes* more and more every day!!!

So, since clear-origin-data puts the burden of matching on the implementer, I am implementing it for localStorage in this bug.

What a surprise!  OriginAttributesPattern don't have PopulateFromSuffix.  Can only be initialized from UTF-16 JSON.  OK, I have to do some UTF conversions here and there.  Also I introduced a sqlite function that does a match of "^appId=1001&inBrowser=1:moc.elpmaxe.:http:80" - which is an example of a value stored in the scope column - against the JSON pattern one gets via the clear-origin-data notification - "{"appId":1001}".  The database update runs on a worker (non-main) thread, of course.  Calling OriginAttributesPattern.Init("{"appId":1001}") asserts...

I'm done.

How can I, please, work with OriginAttributesPattern off the main thread?

    
    

    
  
So, the pattern object is created within the notification (mainthread) and passed down to the database.  That is my workaround.  Makes one of the often used internal classes larger tho.

I think I no longer need help here.
Flags: needinfo?(bobbyholley)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #85) 
> What a surprise!  OriginAttributesPattern don't have PopulateFromSuffix. 

Yeah, a suffix wouldn't make sense given what they do - they're not designed to be attached to an origin, but to represent a pattern of attributes to clear.


> Can only be initialized from UTF-16 JSON.

Yes, per https://dxr.mozilla.org/mozilla-central/rev/11ff0ccb7d59311df4c190d331c8b58c6e35a0c8/caps/BasePrincipal.h#87.

> How can I, please, work with OriginAttributesPattern off the main thread?

Sounds like you've got this part figured out.

    
    

    
  


  
Apply over "DB conversion, v5.1"

- proper "matching" implementation of the clear-origin-data (thanks Yoshi for the Match code example!)
- the origin suffix is added to the database with ":" escaped as "::" so that we can safely find the origin suffix scope prefix to match it ; the database update parts (the 1/2 patch) updated accordingly
- sqlite function that matches the scope against the given pattern (from clear-origin-data)
- new opClearMatchingOrigin DB operation type introduced which carries the OriginAttributesPattern to match against
- this operation is included in the ForgetUpdatesForScope and FindPendingClearForScope optimization filters ; smaug: please look more carefully at there places, those are pretty sensitive

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d517ce7998c

        Attachment #8673243 -
        Flags: review?(bugs)

        Attachment #8673243 -
        Flags: review?(bobbyholley)

    
    

    
  
Comment on attachment 8673243 [details] [diff] [review]
2/2: DOMStorageManager et al use OriginAttributes, v7

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

Overall OriginAttributes handling looks ok - I don't know this code, so I'll defer to smaug's review on the functional level.

::: dom/storage/DOMStorageManager.cpp
@@ +127,5 @@
> +AppendOriginSuffix(nsIPrincipal* aPrincipal,
> +                   nsACString& aKey)
> +{
> +  nsAutoCString suffix;
> +  nsresult rv = aPrincipal->GetOriginSuffix(suffix);

You can use BasePrincipal::Cast(principal)->OriginAttributesRef().CreateSuffix(originSuffix) and skip the rv check.

@@ +148,5 @@
> +  Tokenizer t(aKey);
> +
> +  // This is the char the origin attributes suffix starts with
> +  if (t.CheckChar('^')) {
> +    suffix.Append('^');

The suffix doesn't contain '^' - that's the separator between |originNoSuffix| and |originSuffix|. I don't know why you'd run into it here.

@@ +212,3 @@
>  
> +  // Append the origin suffix, if available
> +  if (!aPrincipal->GetUnknownAppId()) {

The UnknownAppId check isn't necessary anymore post bug 1209843.

@@ +267,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  aKey.Truncate();
> +
> +  if (!aPrincipal->GetUnknownAppId()) {

Same here.

::: dom/storage/DOMStorageObserver.cpp
@@ +202,5 @@
>        return NS_OK;
>      }
>  
> +    nsAutoCString originSuffix;
> +    rv = principal->GetOriginSuffix(originSuffix);

Same BasePrincipal thing here.

        Attachment #8673243 -
        Flags: review?(bobbyholley) → review+

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #89)
> Comment on attachment 8673243 [details] [diff] [review]
> 2/2: DOMStorageManager et al use OriginAttributes, v7
> 
> Review of attachment 8673243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall OriginAttributes handling looks ok - I don't know this code, so I'll
> defer to smaug's review on the functional level.

Thanks!

> > +  if (t.CheckChar('^')) {
> > +    suffix.Append('^');
> 
> The suffix doesn't contain '^' - that's the separator between
> |originNoSuffix| and |originSuffix|. I don't know why you'd run into it here.

According [1], looking into the database and local testing.  However, the check is not needed and if you believe it should not be there, I can remove it.

Looking also few lines bellow [1], it seems like the string should never contain ':'.  Could I remove the '::' escaping?


[1] http://hg.mozilla.org/mozilla-central/annotate/607a236c2299/caps/BasePrincipal.cpp#l93
Flags: needinfo?(bobbyholley)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #90)
> According [1], looking into the database and local testing.  However, the
> check is not needed and if you believe it should not be there, I can remove
> it.

Sorry, I was totally wrong (confirmed by looking at test_origin.js). Ignore my comment. :-P

> Looking also few lines bellow [1], it seems like the string should never
> contain ':'.  Could I remove the '::' escaping?

Yep, looks like it. Though please add a MOZ_RELEASE_ASSERT for mSignedPkg, similar to the mAddonId case. The idea is that we want to check all of the string-valued OriginAttributes (originally it was just mAddonId, but signedPkg was added recently).
Flags: needinfo?(bobbyholley)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        2/2: v7 -> v7.1 (try fixes) (obsolete)
        — 
          — Splinter Review
      

    


  
- DBOperation removes the function rather inside Perform() ; fixes the crash while the worker is in the middle of pattern matching statement execution and the main thread (after shutdown) tries to do other pattern matching which is immediately Finalize()'ed with an error and mistakenly removes the added function
- DBOperation::Finalize() signature reverted back as it was before all these patches
- fixed missing explicit ctor of OriginMatchFunction

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

    
    

    
  


  
Olli, I don't want to obsolete the v7 patch under your hands.  This is however the patch you should finally review.  I unfortunately don't have an interdiff.

This is the latest version, same as v7.1, plus:
- includes review requests from comment 89, 91
- reverts the ":" -> "::" escaping of the origin suffix string in the localStorage database based on comment 91
- nits: fixes constness on some of the getter methods

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

        Attachment #8674268 -
        Flags: review?(bugs)

    
  

        Attachment #8673243 -
        Flags: review?(bugs)

    
  

        Attachment #8673619 -
        Attachment is obsolete: true

    
    

    
  
Sorry, I'm a bit late with this review. I'll try to look at this today.

    
  

        Attachment #8673243 -
        Attachment is obsolete: true

    
    

    
  
Why do we need to pass OriginAttributesPattern objects around?
First we have a string, and using that we initialize a pattern, and then we compare that to
another string, for which we then create OriginAttributes. Odd string parsing and serialization.
Why isn't comparing strings enough?

I guess it is because of the setup we have for clear-origin-data notification.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #95)
> Why do we need to pass OriginAttributesPattern objects around?
> First we have a string, and using that we initialize a pattern, and then we
> compare that to
> another string, for which we then create OriginAttributes. Odd string
> parsing and serialization.
> Why isn't comparing strings enough?

OriginAttributes and OriginAttributesPattern have different semantic meanings (and different serializations). When an OriginAttributes stringification omits a field, that means "the field has the default value". When OriginAttributesPattern stringification omits a field, that means "this matches any value for that field".

    
    

    
  
ok, that isn't clear from any comment I've seen about OriginAttributes*.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #97)
> ok, that isn't clear from any comment I've seen about OriginAttributes*.

Not even this one? http://mxr.mozilla.org/mozilla-central/source/dom/webidl/ChromeUtils.webidl#34

Feel free to add more comments for anything that's not clear.

    
    

    
  
No, that doesn't say what happens if some field is missing from *Pattern. I would have expected
oa and oap which misses some properties wouldn't match in case oa has non-default values.

Though it isn't clear to me why we ever wouldn't want to compare all the properties - so in other words, what is the use case for OriginAttributesPattern?

    
    

    
  
(reading bug 1182347 to understand the background for OAP)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #99)
> No, that doesn't say what happens if some field is missing from *Pattern. I
> would have expected
> oa and oap which misses some properties wouldn't match in case oa has
> non-default values.

From the comment:

> 36  * OriginAttributes, the second is used for pattern-matching (i.e. does this
> 37  * OriginAttributesDictionary match the non-empty attributes in this pattern).

The key words being "match the non-empty attributes". But I totally agree that it could be clearer, and that "non-empty" is probably an inaccurate synonym for "omitted". 
 
> Though it isn't clear to me why we ever wouldn't want to compare all the
> properties - so in other words, what is the use case for
> OriginAttributesPattern?

The use-case is that we sometimes want to clear all the data associated with a given appId - whether or not it's in a browser. The old way of doing this was to send multiple observer notifications, one with {appId=foo, inBrowser=true} and one with {appId=foo, inBrowser=false}. But that doesn't scale with the addition of new OriginAttributes, especially when those attributes are not boolean-valued.  So OriginAttributesPattern lets you Say What You Mean in that case.

    
    

    
  
Comment on attachment 8674268 [details] [diff] [review]
2/2: DOMStorageManager et al use OriginAttributes, v7.2

>@@ -114,87 +118,93 @@ public:
>       opAddItem,
>       opUpdateItem,
>       opRemoveItem,
>       opClear,
> 
>       // Operations invoked by chrome
>       opClearAll,
>       opClearMatchingScope,
>+      opClearMatchingOrigin,
>     } OperationType
I'd like to see some comment what is the difference between 
opClearMatchingScope and opClearMatchingOrigin and why we need them both
and actually why also opClear.
Why only opClear needs to call mScopesHavingData.RemoveEntry()?

>+void
>+AppendOriginSuffix(nsIPrincipal* aPrincipal, nsACString& aKey)
>+{
>+  nsAutoCString suffix;
>+  BasePrincipal::Cast(aPrincipal)->OriginAttributesRef().CreateSuffix(suffix);
>+
>+  aKey.Append(suffix);
>+}
Why we need this method? Shouldn't 
principal->GetOriginSuffix(key); work just as well.
You seem to Truncate the param string anyway always before calling the method.



>@@ -197,32 +197,42 @@ DOMStorageObserver::Observe(nsISupports*
>     }
> 
>     nsCOMPtr<nsIPrincipal> principal;
>     perm->GetPrincipal(getter_AddRefs(principal));
>     if (!principal) {
>       return NS_OK;
>     }
> 
>+    nsAutoCString originSuffix;
>+    BasePrincipal::Cast(principal)->OriginAttributesRef().CreateSuffix(originSuffix);
>+
>     nsCOMPtr<nsIURI> origin;
>     principal->GetURI(getter_AddRefs(origin));
>     if (!origin) {
>       return NS_OK;
>     }
> 
>     nsAutoCString host;
>     origin->GetHost(host);
>     if (host.IsEmpty()) {
>       return NS_OK;
>     }
> 
>     nsAutoCString scope;
>-    rv = CreateReversedDomain(host, scope);
>+
>+    scope.Append(originSuffix);
>+
>+    nsAutoCString reversedDomain;
>+    rv = CreateReversedDomain(host, reversedDomain);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>+    scope.Append(":");
>+    scope.Append(reversedDomain);
>+
>     Notify("session-only-cleared", scope);
> 
>     return NS_OK;
>   }
> 
>   // Clear everything (including so and pb data) from caches and database
>   // for the gived domain and subdomains.
>   if (!strcmp(aTopic, "browser:purge-domain-data")) {
>@@ -234,73 +244,57 @@ DOMStorageObserver::Observe(nsISupports*
>       NS_ENSURE_SUCCESS(rv, rv);
>     } else {
>       // In case the IDN service is not available, this is the best we can come up with!
>       NS_EscapeURL(NS_ConvertUTF16toUTF8(aData),
>                    esc_OnlyNonASCII | esc_AlwaysCopy,
>                    aceDomain);
>     }
> 
>-    nsAutoCString scopePrefix;
>-    rv = CreateReversedDomain(aceDomain, scopePrefix);
>+    nsAutoCString scope;
>+
>+    nsAutoCString reversedDomain;
>+    rv = CreateReversedDomain(aceDomain, reversedDomain);
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>+    scope.Append(":");
>+    scope.Append(reversedDomain);
>+
>     DOMStorageDBBridge* db = DOMStorageCache::StartDatabase();
>     NS_ENSURE_TRUE(db, NS_ERROR_FAILURE);
> 
>-    db->AsyncClearMatchingScope(scopePrefix);
>+    db->AsyncClearMatchingScope(scope);
> 
>-    Notify("domain-data-cleared", scopePrefix);
>+    Notify("domain-data-cleared", scope);
I don't understand why OriginAttributesPattern (or would it be OriginAttributes?) isn't used in the scope.
The code above dealing with "perm-changed" does use OriginAttributes in the scope.

This needs some comments why we're strict in some cases but don't care about OriginAttributes at all in some other cases.


r- because of some missing comments and explanations.

        Attachment #8674268 -
        Flags: review?(bugs) → review-

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #102)
> Comment on attachment 8674268 [details] [diff] [review]
> 2/2: DOMStorageManager et al use OriginAttributes, v7.2
> 
> >@@ -114,87 +118,93 @@ public:
> >       opAddItem,
> >       opUpdateItem,
> >       opRemoveItem,
> >       opClear,
> > 
> >       // Operations invoked by chrome
> >       opClearAll,
> >       opClearMatchingScope,
> >+      opClearMatchingOrigin,
> >     } OperationType
> I'd like to see some comment what is the difference between 
> opClearMatchingScope and opClearMatchingOrigin and why we need them both
> and actually why also opClear.
> Why only opClear needs to call mScopesHavingData.RemoveEntry()?

mScopesHavingData hashtable is just and only an optimization.  It prevents an early preload of a localStorage cache when there are no data under the scope.  Not clearing it will have a negligible effect.  However, this explanation is missing in the code.

I can introduce a code that would drop scopes from that hashtable matching the scope to clear.  Or I can add an explanation to this patch why there is no strong need to prune it and clear it in a followup bug.

> 
> >+void
> >+AppendOriginSuffix(nsIPrincipal* aPrincipal, nsACString& aKey)
> >+{
> >+  nsAutoCString suffix;
> >+  BasePrincipal::Cast(aPrincipal)->OriginAttributesRef().CreateSuffix(suffix);
> >+
> >+  aKey.Append(suffix);
> >+}
> Why we need this method? Shouldn't 
> principal->GetOriginSuffix(key); work just as well.
> You seem to Truncate the param string anyway always before calling the
> method.

There used to be more code in this function, probably worth removing it.

> >@@ -234,73 +244,57 @@ DOMStorageObserver::Observe(nsISupports*
> >       NS_ENSURE_SUCCESS(rv, rv);
> >     } else {
> >       // In case the IDN service is not available, this is the best we can come up with!
> >       NS_EscapeURL(NS_ConvertUTF16toUTF8(aData),
> >                    esc_OnlyNonASCII | esc_AlwaysCopy,
> >                    aceDomain);
> >     }
> > 
> >-    nsAutoCString scopePrefix;
> >-    rv = CreateReversedDomain(aceDomain, scopePrefix);
> >+    nsAutoCString scope;
> >+
> >+    nsAutoCString reversedDomain;
> >+    rv = CreateReversedDomain(aceDomain, reversedDomain);
> >     NS_ENSURE_SUCCESS(rv, rv);
> > 
> >+    scope.Append(":");
> >+    scope.Append(reversedDomain);
> >+
> >     DOMStorageDBBridge* db = DOMStorageCache::StartDatabase();
> >     NS_ENSURE_TRUE(db, NS_ERROR_FAILURE);
> > 
> >-    db->AsyncClearMatchingScope(scopePrefix);
> >+    db->AsyncClearMatchingScope(scope);
> > 
> >-    Notify("domain-data-cleared", scopePrefix);
> >+    Notify("domain-data-cleared", scope);
> I don't understand why OriginAttributesPattern (or would it be
> OriginAttributes?) isn't used in the scope.
> The code above dealing with "perm-changed" does use OriginAttributes in the
> scope.

There is no good documentation on the "browser:purge-domain-data" notification except a line: "Sent after domain-specific history and other information have been purged. The data value is a string form of the domain."  

What exactly to do, delete only "common" web data, or delete anything stored by that domain or something even more specific is not expressed.  The original code (unpatched) and I believe even the old DOM storage code (before my rewrite) did exactly this: any origin prefix has been ignored and those data (stored by apps mostly) were left.

So, how exactly this notification has to be handled?
Flags: needinfo?(bugs)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #103)
> (In reply to Olli Pettay [:smaug] from comment #102)
> > Comment on attachment 8674268 [details] [diff] [review]
> > 2/2: DOMStorageManager et al use OriginAttributes, v7.2
> > 
> > >@@ -114,87 +118,93 @@ public:
> > >       opAddItem,
> > >       opUpdateItem,
> > >       opRemoveItem,
> > >       opClear,
> > > 
> > >       // Operations invoked by chrome
> > >       opClearAll,
> > >       opClearMatchingScope,
> > >+      opClearMatchingOrigin,
> > >     } OperationType
> > I'd like to see some comment what is the difference between 
> > opClearMatchingScope and opClearMatchingOrigin and why we need them both
> > and actually why also opClear.
> > Why only opClear needs to call mScopesHavingData.RemoveEntry()?
> 
> mScopesHavingData hashtable is just and only an optimization.  It prevents
> an early preload of a localStorage cache when there are no data under the
> scope.  Not clearing it will have a negligible effect.  However, this
> explanation is missing in the code.
Yes, please add such comment. And please add also some comment explaining
the difference of opClear, opClearMatchingScope and opClearMatchingOrigin.
Especially the difference between opClearMatchingScope and opClearMatchingOrigin isn't too clear atm.

> I can introduce a code that would drop scopes from that hashtable matching
> the scope to clear.  Or I can add an explanation to this patch why there is
> no strong need to prune it and clear it in a followup bug.
explanation is enough


> There is no good documentation on the "browser:purge-domain-data"
> notification except a line: "Sent after domain-specific history and other
> information have been purged. The data value is a string form of the
> domain."  
> 
> What exactly to do, delete only "common" web data, or delete anything stored
> by that domain or something even more specific is not expressed.  The
> original code (unpatched) and I believe even the old DOM storage code
> (before my rewrite) did exactly this: any origin prefix has been ignored and
> those data (stored by apps mostly) were left.
> 
> So, how exactly this notification has to be handled?
Looks like http://mxr.mozilla.org/mozilla-central/source/toolkit/forgetaboutsite/ForgetAboutSite.jsm#170 fires the notification.
And since we haven't cared about isBrowser or other stuff before, maybe no need to change that in this bug, but
could you file a followup bug to make sure browser:purge-domain-data does what it is supposed to do.
Flags: needinfo?(bugs)

    
    

    
  
I think we should solve what to delete in these notifications exactly now, before we update the database, since more changes to the "scope key" may be needed.

To explain: the "scope key", created by CreateScopeKey function, contains four parts, in the exact order: the origin attributes suffix string, the domain (in a reversed order), schema and port.

The goal is to have a string that goes from "most general" to "most specific".  Using a simple LIKE and % notation you easily delete from the database anything that falls under a domain + all its subdomains.

Question here is, whether the origin attributes should be something "most general" or rather "most specific" - i.e. at the end of the string.

I can imagine that deleting everything under a domain should include anything stored also by apps under that domain.

But, also don't forget that OriginAttributes is something more generic, it may also express a non-null user context or anything we invent in the future that has nothing to do with apps and appids.  

The user context id (for instance) will be put to the start of the string, as something "most general".  And when we request deletion of domain-specific data, those prefixed with a user context id will not be deleted (with the patch 1/2-v5.1 and 2/2-v7.2).  I cannot answer if we should or not.

Jonas, based on the discussion above, should the origin suffix be at the beginning of the scope key or at the end of it?
Flags: needinfo?(jonas)

    
    

    
  
So, would it be correct to say that the question is equivalent to:

Will we run more queries like "Delete all data for domain x.y.z, independent of what the origin attributes are", or will we run more queries like "Delete all data for origin attributes { x, y, z }, independent of the domain of the data".

Do I understand correctly?

If so, I would start by saying that I don't think we really know. I certainly don't. It depends entirely on what UX features that the Firefox team decides to create that use origin attributes, and what types of privacy features that they end up building.

For example, I know that they are planning to build a feature which allows you to create a "work context" and a "private context". The two would get different cookie jars through using origin attributes. Presumably there will be a function to create and delete such contexts, which probably would delete any and all data in that context. I.e. all data for a given origin attribute, independent of domain. In that case origin attributes would be "more general".

But I don't know how it will interact with privacy features that I think we already have which allows clearing data for a given website. I don't know if that will clear data for all contexts, in which case domain would be "more general" there.


If I had to guess, more often than not origin attributes will likely be more general than domain. But I would probably put the two in separate columns so that we can write queries to run either of the queries above. It would mean that in one case we would have to do a full table scan, but at least we wouldn't have to parse the data on each row to extract the domain.
Flags: needinfo?(jonas)

    
    

    
  
Jonas, thanks.

To have two columns seems a good solution to me too.  The origin and the domain are mostly orthogonal.  As you say, it's hard to know in advance which of the two is more general.

It also opens possibility to delete by a certain origin pattern AND a domain (+subdomains).

I'll update the patch accordingly.

    
    

    
  


  
WIP backup of the split of the |scope| column to |origin-suffix| and |origin-key| columns.  Also means to split all internal apis working with just a |scope| key to two strings.  Lot of refactoring, but actually just a mechanical change.  This patch has never run, just builds on win.

    
    

    
  
Note: this patch reacts to "browser:purge-domain-data" notification by deleting all data under a domain+subdomains.  The OriginAttributes part is ignored, so that any appdata go away too.

    
  
QA Whiteboard: [COM=NSec]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        vII.1 (obsolete)
        — 
          — Splinter Review
      

    


  
- rather merged to a single patch
- I moved the database update code to a separate cpp/h ; I was not using hg cp eventually because after few more changes the patch for the new file looked pretty unreadable
- data are replaced to a new table "domstorage_3", columns "secret" and "owner" dropped too, those were no longer used
- doing vacuum to save significant number of disk space after the update (note that the update is performed after we initially render the window, so that it doesn't block UI showing up at the start, this has not changed with this patch)
- "scope" column split to "originAttributes" and "originKey"
- originAttributes is obviously the origin attributes suffix
- originKey is reversed domain ":" schema ":" port
- the internal API including IPC is split to work separately with originAttributes and originKey (huge change, but actually pretty mechanical)
- for function arguments I often use notation just as "suffix" and "origin" for those two to save typing and readability
- browser:purge-domain-data notification deletes everything stored under a domain + subdomains, regardless the origin attributes <- the biggest behavioral change of this patch
- perm-changed notification now respects origin attributes
- "scope" (=suffix+origin) property left on DOMStorageCache since it's useful to scope via a single string rather then comparing both suffix and origin on few places and makes the patch smaller
- OriginAttributes::mSignedPkg assertion about replace chars added
- usage scope is semantically left as before this patch (origin attribs ":" eTLD+1 reversed domain), it works as it has to this way
- an important stability fix: mScopesHavingData hashtable was being accessed concurrently w/o locking!  added monitor lock over it

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22c93648e156

        Attachment #8661252 -
        Attachment is obsolete: true

        Attachment #8674268 -
        Attachment is obsolete: true

        Attachment #8678503 -
        Attachment is obsolete: true

        Attachment #8679463 -
        Flags: review?(bugs)

        Attachment #8679463 -
        Flags: review?(bobbyholley)

    
    

    
  
Comment on attachment 8679463 [details] [diff] [review]
vII.1

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

I'm busy with other things, and I think smaug's review is sufficient here. Please let me know if there's anything specific you want me to look at.

        Attachment #8679463 -
        Flags: review?(bobbyholley)

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #111)
> Comment on attachment 8679463 [details] [diff] [review]
> vII.1
> 
> Review of attachment 8679463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm busy with other things, and I think smaug's review is sufficient here.
> Please let me know if there's anything specific you want me to look at.

OK, I think the only thing you may check on is how this patch handles the deletion observer notifications.  But I think even in that case smaug's review can be taken as sufficient.  Thanks.

    
    

    
  
Comment on attachment 8679463 [details] [diff] [review]
vII.1

dropping r.  apparently the patch also needs to preserve downgrade compatibility of the database.  Unfortunate, but needed.

        Attachment #8679463 -
        Flags: review?(bugs)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #113)
> Comment on attachment 8679463 [details] [diff] [review]
> vII.1
> 
> dropping r.  apparently the patch also needs to preserve downgrade
> compatibility of the database.  Unfortunate, but needed.

So, there is a drawback with this...  I've introduced a new unique index for (originSuffix, originKey, key) tuple in schema 1 on the recreated webappsstore2 table.  It's vital for INSERT OR REPLACE command.  However, when someone returns to schema 0, this index will still be there.  And as we don't fill originKey and originSuffix when running the old code, INSERT OR REPLACE may fail when there is an overlap.  Also, should there be an overlap for |scope + key| in the database (it can, since scope doesn't keep all the values of OriginAttributes) recreation of scope_key_index on downgrade can easily fail and the data will be inaccessible.  Database should tho survive and be again accessible after update to schema 1 code.

Both issue can happen with just a low probability and cannot happen for common (default OA) web content at all.  So I don't think it would be a big problem.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        vII.2 (obsolete)
        — 
          — Splinter Review
      

    


  
- I left the webappsstore2 sqlite table name
- we preserve the 'scope' column to be able to go back to older versions (and back) w/o loosing data
- there is some dance and drawbacks with indexes, should be explained in comments in the code and also in bugzilla (comment 114)

        Attachment #8679463 -
        Attachment is obsolete: true

        Attachment #8681286 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8681286 [details] [diff] [review]
vII.2


>+++ b/dom/storage/PStorage.ipdl
>@@ -14,30 +14,32 @@ namespace dom {
>  */
> prio(normal upto urgent) sync protocol PStorage
> {
>   manager PContent;
> 
> parent:
>   async __delete__();
> 
>-  prio(urgent) sync Preload(nsCString scope, uint32_t alreadyLoadedCount)
>+  prio(urgent) sync Preload(nsCString suffix, nsCString origin, uint32_t alreadyLoadedCount)
>     returns (nsString[] keys, nsString[] values, nsresult rv);
> 
>-  async AsyncPreload(nsCString scope, bool priority);
>+  async AsyncPreload(nsCString suffix, nsCString origin, bool priority);
So I don't understand all this suffix + origin sending. origin should contain suffix (per nsIPrincipal terminology), so why are we explicitly
sending also suffix?

 
Similar naming issue is OriginKey(). I don't understand what it means.
I think it is "originNoSuffix"
And Scope() is then what nsIPrincipal calls origin.

Using different terminology than nsIPrincipal makes this hard to review.

        Attachment #8681286 -
        Flags: review?(bugs) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        vII.3 (obsolete)
        — 
          — Splinter Review
      

    


  
- rebased
- 'originKey' and 'origin' renamed to 'originNoSuffix' (holding the host+schema+port tuple in the special database key format)
- 'suffix' renamed to 'originSuffix' (origin attributes suffix, result of OriginAttributes::CreateSuffix)
- 'scope' renamed to 'origin' (concatenation of originNoSuffix and originSuffix, used on some places)
- I left the new database columns unrenamed since the current names more express what's going on
- usage quota key and also scope prefix (to match with StringBeginsWith on originNoSuffix) renamed to 'originScope', feel free to suggest a better name

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50d2f7ab49d8

        Attachment #8681286 -
        Attachment is obsolete: true

        Attachment #8695989 -
        Flags: review?(bugs)

    
    

    
  
So does mOriginsHavingData contain origins or originNoSuffix' ?
DOMStorageDBThread::ShouldPreloadOrigin(const nsACString& aOriginNoSuffix) hints *NoSuffix
but DOMStorageDBThread::GetOriginsHavingData(InfallibleTArray<nsCString>* aOrigins) hints about
it containing origins.
The comment about the variable says just "// List of scopes having data, for optimization purposes only"

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #120)
> So does mOriginsHavingData contain origins or originNoSuffix' ?

Full origin.

> DOMStorageDBThread::ShouldPreloadOrigin(const nsACString& aOriginNoSuffix)
> hints *NoSuffix

Bad rename, will update.

> but DOMStorageDBThread::GetOriginsHavingData(InfallibleTArray<nsCString>*
> aOrigins) hints about
> it containing origins.

That one is correct.

> The comment about the variable says just "// List of scopes having data, for
> optimization purposes only"

Will update in the next patch version as:

  // List of origins (including origin attributes suffix) having data, for optimization purposes only


Do you want a new patch with these small nits fixed for review or will you go on with the current version and I will update afterwards?

    
    

    
  
no need.
thanks for the clarification.

    
    

    
  
Comment on attachment 8695989 [details] [diff] [review]
vII.3

> DOMStorageCache::Init(DOMStorageManager* aManager,
>                       bool aPersistent,
>                       nsIPrincipal* aPrincipal,
>-                      const nsACString& aQuotaScope)
>+                      const nsACString& aQuotaOriginScope)
> {
>   if (mInitialized) {
>     return;
>   }
> 
>   mInitialized = true;
>   mPrincipal = aPrincipal;
>+  BasePrincipal::Cast(aPrincipal)->OriginAttributesRef().CreateSuffix(mOriginSuffix);
>   mPersistent = aPersistent;
>-  mQuotaScope = aQuotaScope.IsEmpty() ? mScope : aQuotaScope;
>+  if (aQuotaOriginScope.IsEmpty()) {
>+    mQuotaOriginScope = Origin();
>+  } else {
>+    mQuotaOriginScope = aQuotaOriginScope;
>+  }
Why origin suffix is taken from principal but mQuotaOriginScope is from string?
Partially old code but a bit surprising. Some comment here might be useful.
Is aQuotaOriginScope in reversed format? I think it is. Could use a comment.
Could we at least assert in debug code what mQuotaOriginScope has the same suffix as principal?



>-  explicit DOMStorageCache(const nsACString* aScope);
>+  explicit DOMStorageCache(const nsACString* aOriginNoSuffix);
> 
> protected:
>   virtual ~DOMStorageCache();
> 
> public:
>   void Init(DOMStorageManager* aManager, bool aPersistent, nsIPrincipal* aPrincipal,
>-            const nsACString& aQuotaScope);
>+            const nsACString& aQuotaOriginScope);

This all is rather hard to follow. The ctor takes some origin without suffix, Init takes principal
(which has proper origin and all) and it takes also aQuotaOriginScope which is eTLD+1 in reversed form (I think).
Why do we need to pass originNoSuffix to ctor? Could use some comment.
>+Scheme0Scope(DOMStorageCacheBridge *aCache)
Nit, * goes with the type, not with variable name


+  // The origin of the cache
+  virtual const nsCString Origin() const = 0;
In what kind of syntax? This is in reversed format + suffix?

Is there any chance to pass principal around and just take origin and suffix from it when needed?

I think I'd like to see a new patch with some more comments. (and if possible, more principal usage, or if that isn't possible, explanation why not. Sorry, this is complicated stuff.)

        Attachment #8695989 -
        Flags: review?(bugs)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #123)
> Comment on attachment 8695989 [details] [diff] [review]
> vII.3
> 
> > DOMStorageCache::Init(DOMStorageManager* aManager,
> >                       bool aPersistent,
> >                       nsIPrincipal* aPrincipal,
> >-                      const nsACString& aQuotaScope)
> >+                      const nsACString& aQuotaOriginScope)
> > {
> >   if (mInitialized) {
> >     return;
> >   }
> > 
> >   mInitialized = true;
> >   mPrincipal = aPrincipal;
> >+  BasePrincipal::Cast(aPrincipal)->OriginAttributesRef().CreateSuffix(mOriginSuffix);
> >   mPersistent = aPersistent;
> >-  mQuotaScope = aQuotaScope.IsEmpty() ? mScope : aQuotaScope;
> >+  if (aQuotaOriginScope.IsEmpty()) {
> >+    mQuotaOriginScope = Origin();
> >+  } else {
> >+    mQuotaOriginScope = aQuotaOriginScope;
> >+  }
> Why origin suffix is taken from principal but mQuotaOriginScope is from
> string?

I don't much understand the question.  mQuotaOriginScope is something different.  It's a "NoSuffix" reversed-format origin that is used to math domain and subdomains of an origin, origin attributes are deliberately ignored.  If you want this changed, then in a different bug please.  Also, localStorage will soon move QuataManager, hence this all will radically change once more soon.

> Partially old code but a bit surprising. Some comment here might be useful.
> Is aQuotaOriginScope in reversed format? 

Yes, I'll comment on that.

> I think it is. Could use a comment.
> Could we at least assert in debug code what mQuotaOriginScope has the same
> suffix as principal?

No, since the quota origin scope is intended to ignore the suffix.

> 
> 
> 
> >-  explicit DOMStorageCache(const nsACString* aScope);
> >+  explicit DOMStorageCache(const nsACString* aOriginNoSuffix);
> > 
> > protected:
> >   virtual ~DOMStorageCache();
> > 
> > public:
> >   void Init(DOMStorageManager* aManager, bool aPersistent, nsIPrincipal* aPrincipal,
> >-            const nsACString& aQuotaScope);
> >+            const nsACString& aQuotaOriginScope);
> 
> This all is rather hard to follow. The ctor takes some origin without
> suffix, Init takes principal
> (which has proper origin and all) and it takes also aQuotaOriginScope which
> is eTLD+1 in reversed form (I think).
> Why do we need to pass originNoSuffix to ctor? Could use some comment.

Because the cache is created from inside a custom hash-entry class for which the origin-no-suffix is used as a key.  Hence it's easy to pass via the cache's ctor.  See http://hg.mozilla.org/mozilla-central/annotate/9aa2dae27ae5/dom/storage/DOMStorageManager.h#l57

> +  // The origin of the cache
> +  virtual const nsCString Origin() const = 0;
> In what kind of syntax? This is in reversed format + suffix?

Yep, will comment.

> 
> Is there any chance to pass principal around and just take origin and suffix
> from it when needed?

I'm not passing pincipal from the very first implementation.  The reason is that if you want the reversed-format origin (no-suffix) you need to always reconvert when you have only principal in hands.  To keep it as a string is a form performance caching.

> 
> I think I'd like to see a new patch with some more comments. (and if
> possible, more principal usage, or if that isn't possible, explanation why
> not. Sorry, this is complicated stuff.)

Yep, even a simple tech as localStorage is complicated :D  

Olli, thanks for so fast responses, it makes me believe this growing patch is going to land soon.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        vII.4 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8695989 -
        Attachment is obsolete: true

        Attachment #8697137 -
        Flags: review?(bugs)

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #124)

> > Why origin suffix is taken from principal but mQuotaOriginScope is from
> > string?
> 
> I don't much understand the question.  mQuotaOriginScope is something
> different.  It's a "NoSuffix" reversed-format origin that is used to math
> domain and subdomains of an origin, origin attributes are deliberately
> ignored.  If you want this changed,
No no, just trying to understand the code. I can see by looking at the other code that mQuotaOriginScope isn't
any origin but it is eTLD+1 scope, but this code could use some comment, and also, it might be good to explain why
we can't get the originNoSuffifx from principal and transform it to eTLD+1 scope here? Why we need to pass
strings here as param.  So mostly just wanting to see some comments in the code and perhaps thinking about the option
to pass principal around and not strings (passing a principal would be probably less error prone, or perhaps some struct which has principal and various string versions of the origin cached)..


> > I think it is. Could use a comment.
> > Could we at least assert in debug code what mQuotaOriginScope has the same
> > suffix as principal?
> 
> No, since the quota origin scope is intended to ignore the suffix.
Hmm, I wonder why. Why don't we want eTLD+1 scope + suffix? Why only eTLD+1 scope?


Will look at the latest patch tomorrow.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #126)
> (In reply to Honza Bambas (:mayhemer) from comment #124)
> 
> > > Why origin suffix is taken from principal but mQuotaOriginScope is from
> > > string?
> > 
> > I don't much understand the question.  mQuotaOriginScope is something
> > different.  It's a "NoSuffix" reversed-format origin that is used to math
> > domain and subdomains of an origin, origin attributes are deliberately
> > ignored.  If you want this changed,
> No no, just trying to understand the code. I can see by looking at the other
> code that mQuotaOriginScope isn't
> any origin but it is eTLD+1 scope, but this code could use some comment, and
> also, it might be good to explain why
> we can't get the originNoSuffifx from principal and transform it to eTLD+1
> scope here? Why we need to pass
> strings here as param.  So mostly just wanting to see some comments in the
> code and perhaps thinking about the option
> to pass principal around and not strings (passing a principal would be
> probably less error prone, or perhaps some struct which has principal and
> various string versions of the origin cached)..

It's about code cleanness, simpleness and performance.  The functions creating these strings are in DOMStorageManager.cpp, close together, so it's easy to inspect them and no need to export them.  To pass around the principal may end up by moving these functions elsewhere or even export them.  However, I would not loose time on these details.  This is something this bug is not about to change.  The goal is to use origin suffix, no cleanup or anything.  And I think the patch is already complex enough..

> 
> 
> > > I think it is. Could use a comment.
> > > Could we at least assert in debug code what mQuotaOriginScope has the same
> > > suffix as principal?
> > 
> > No, since the quota origin scope is intended to ignore the suffix.
> Hmm, I wonder why. Why don't we want eTLD+1 scope + suffix? Why only eTLD+1
> scope?

Oh!  I was wrong!  It does contain the suffix.  And it's just a single string, compared to concatenation of suffix and origin-no-suffix (see DOMStorageDBThread::DBOperation::Perform, opGetUsage case).  So, maybe just add a nice comment?  I really don't want to play with names here even more...

> 
> 
> Will look at the latest patch tomorrow.

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #127)
> > > > I think it is. Could use a comment.
> > > > Could we at least assert in debug code what mQuotaOriginScope has the same
> > > > suffix as principal?
> > > 
> > > No, since the quota origin scope is intended to ignore the suffix.
> > Hmm, I wonder why. Why don't we want eTLD+1 scope + suffix? Why only eTLD+1
> > scope?
> 
> Oh!  I was wrong!  It does contain the suffix.  And it's just a single
> string, compared to concatenation of suffix and origin-no-suffix (see
> DOMStorageDBThread::DBOperation::Perform, opGetUsage case).  So, maybe just
> add a nice comment?  I really don't want to play with names here even more...

Well, assertion would be nice there. mQuotaOriginScope should have the same suffix stuff as the principal, right?
Such assertion would self document the code.

    
    

    
  
Comment on attachment 8697137 [details] [diff] [review]
vII.4

> DOMStorageCache::Init(DOMStorageManager* aManager,
>                       bool aPersistent,
>                       nsIPrincipal* aPrincipal,
>-                      const nsACString& aQuotaScope)
>+                      const nsACString& aQuotaOriginScope)
> {
>   if (mInitialized) {
>     return;
>   }
> 
>   mInitialized = true;
>   mPrincipal = aPrincipal;
>+  BasePrincipal::Cast(aPrincipal)->OriginAttributesRef().CreateSuffix(mOriginSuffix);
>   mPersistent = aPersistent;
>-  mQuotaScope = aQuotaScope.IsEmpty() ? mScope : aQuotaScope;
>+  if (aQuotaOriginScope.IsEmpty()) {
>+    mQuotaOriginScope = Origin();
>+  } else {
>+    mQuotaOriginScope = aQuotaOriginScope;
>+  }
So I think this code should assert in debug builds that mOriginSuffix is found in
mQuotaOriginScope, and that if mQuotaOriginScope has some suffix stuff in it, it is the same as mOriginSuffix
(so doing checks in both ways).

>+  // The eTLD+1 scope used to count quota usage.  It is in the reversed format and never
>+  // contains the origin attributes suffix, quota is calculated for anything falling under
>+  // an origin-no-suffix scope.
>+  nsCString mQuotaOriginScope;
This comment is wrong, right? It does contain suffix

>+namespace {
>+
>+class OriginMatchSQLFunction final : public mozIStorageFunction
This naming is odd. The function isn't about origin matching, but about suffix matching.


>+  case opClearMatchingOriginAttributes:
>+  {
>+    MOZ_ASSERT(!NS_IsMainThread());
>+
>+    // Register the ORIGIN_MATCH function, initilized with the pattern
initialized


>+    nsCOMPtr<mozIStorageFunction> originMatchFunction(new OriginMatchSQLFunction(mOriginPattern));
>+    rv = aThread->mWorkerConnection->CreateFunction(
>+      NS_LITERAL_CSTRING("ORIGIN_MATCH"), 1, originMatchFunction);
>+    NS_ENSURE_SUCCESS(rv, rv);
So why ORIGIN_MATCH, when we're dealing with origin attributes here.
Sorry, I'm rather strict with naming here, but it is rather critical from readability point of view.


+ExtractOriginData(const nsACString& scope, nsACString& suffix, nsACString& origin)
aScope, aSuffix, aOrigin




+  while (tokenizer.Next(t)) {
+  }
Just curious, tokenizer really doesn't have a way to go to the end?

>+  // Clear data of the origins whose prefixes will match the suffix.
>+  if (!strcmp(aTopic, "clear-origin-data")) {
>+    OriginAttributesPattern pattern;
>+    if (!pattern.Init(nsDependentString(aData))) {
>+      NS_ERROR("Cannot parse origin attributes pattern");
>+      return NS_ERROR_FAILURE;
...
>+    db->AsyncClearMatchingOriginAttributes(pattern);
...
>+    Notify("origin-data-cleared", nsDependentString(aData));
So this isn't about origin, but origin attributes. Some better notification name is needed for this, I think.
What happens if aData is empty string?

        Attachment #8697137 -
        Flags: review?(bugs) → review-

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #129)
> >+  // Clear data of the origins whose prefixes will match the suffix.
> >+  if (!strcmp(aTopic, "clear-origin-data")) {
> >+    OriginAttributesPattern pattern;
> >+    if (!pattern.Init(nsDependentString(aData))) {
> >+      NS_ERROR("Cannot parse origin attributes pattern");
> >+      return NS_ERROR_FAILURE;
> ...
> >+    db->AsyncClearMatchingOriginAttributes(pattern);
> ...
> >+    Notify("origin-data-cleared", nsDependentString(aData));
> So this isn't about origin, but origin attributes. Some better notification
> name is needed for this, I think.

"origin-data-cleared" is an internal DOM storage notification to handle "clear-origin-data" global notification.  What better name for christ sake it should have?

> What happens if aData is empty string?

I don't care.  As any other implementer of this notification.  If there is a problem with it, we should solve it a different bug.

    
    

    
  


  
For simpler review of some parts.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        A test update (obsolete)
        — 
          — Splinter Review
      

    


  
Apparently, this patch also fixes a TODO check in a recently introduced test for user context.  We need to update that test too!

        Attachment #8699127 -
        Flags: review?(bugs)

        Attachment #8699127 -
        Flags: review?(bugs) → review+

    
    

    
  
Comment on attachment 8699053 [details] [diff] [review]
vII.4 -> 5 idiff

OriginAttribsPatternMatchSQLFunction sounds a tad odd.
Attribute is more often shorten to Attr in Gecko.
OriginAttrsPatternMatchSQLFunction?

ORIGIN_ATTRIBS_PATTERN_MATCH -> ORIGIN_ATTRS_PATTERN_MATCH

I still think clear-origin-data is wrong name since it isn't about clearing origin data.
As the comment says it is about
"+  // Clear data of the origins whose prefixes will match the suffix."
Could it be clear-origin-attribute-data ?
We need to be careful with naming to reduce the risk for people misusing the notifications and also
using precise naming makes code easier to read.

With those, r+

        Attachment #8699053 -
        Flags: review?(bugs) → review+

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #134)
> I still think clear-origin-data is wrong name since it isn't about clearing
> origin data.
> As the comment says it is about
> "+  // Clear data of the origins whose prefixes will match the suffix."
> Could it be clear-origin-attribute-data ?
> We need to be careful with naming to reduce the risk for people misusing the
> notifications and also
> using precise naming makes code easier to read.
> 
> With those, r+

"clear-origin-data" global observer notification is out of my control, right?  This is not something I've introduced.  If you don't like it, file a new bug to do an overall-code-base mass rename of it.  I don't think people are going to be happy about it tho, as it's widely used now.

I can only rename my internal notification name "origin-data-cleared" to something that will make you more happy tho.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        vII.6
        — 
          — Splinter Review
      

    


  
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1678c3783bdd

        Attachment #8699053 -
        Attachment is obsolete: true

        Attachment #8699055 -
        Attachment is obsolete: true

        Attachment #8699127 -
        Attachment is obsolete: true

        Attachment #8704174 -
        Flags: review+

    
  
Keywords: checkin-needed

    
    

    
  
(In reply to Honza Bambas (:mayhemer) from comment #135)
> "clear-origin-data" global observer notification is out of my control,
> right?
How so? Just change the name as it was changed in 
https://bugzilla.mozilla.org/show_bug.cgi?id=1182347

But fine, if you file a followup bug to change the notification name. CC bholley and me to that bug.
The use of webapps-clear-data and clear-origin-data is such a mess. First one clears only some relevant data, and the latter one doesn't clear origin data but something else.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #137)
> (In reply to Honza Bambas (:mayhemer) from comment #135)
> > "clear-origin-data" global observer notification is out of my control,
> > right?
> How so? Just change the name as it was changed in 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1182347

Sorry, I don't follow what you want from me.

    
    

    
  
just file a followup to clean up that mess.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #139)
> just file a followup to clean up that mess.

Bug 1237152

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/35794bec70ba
Status: ASSIGNED → RESOLVED
Closed: 9 years ago

          status-firefox46:
          --- → fixed
Resolution: --- → FIXED

    
  
Depends on: 1240238

    
  
Depends on: 1244038
Depends on: 1251026

    
  
Regressions: 1676410

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: