Closed Bug 1165263 Opened 9 years ago Closed 9 years ago

Use origin for nsIPermissionManager

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: allstars.chh, Assigned: nika)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(8 files, 9 obsolete files)

Part 6: Update browser tests to support new permissions semantics
2.72 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 4: Update reftest runners to support new moz_hosts schema
2.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Part 3: Update mozprofile to support new moz_hosts schema
9.67 KB, patch
ahal
: review+
Details | Diff | Splinter Review
Part 2: Update unit tests for nsPermissionManager to reflect new internals
27.23 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 1: Update nsPermissionManager to use origins instead of hosts internally
66.29 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
part1_v3-v5_interdiff.diff
19.15 KB, text/plain
Details
Part 5: Update PermissionsUtils.importPrefBranch to use origins instead of hosts
12.20 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 7: Update tracking protection to account for new nsPermissionManager semantics
3.51 KB, patch
Margaret
: review+
Details | Diff | Splinter Review 
We implemented permission for apps on B2G in Bug 786296 and Bug 786299.
Now for the new security model we will update the nsIPrincipal.origin in Bug 1163254.
So nsIPermissionManager should use the origin instead of using appId/isBrowser.
(In reply to Yoshi Huang[:allstars.chh] from comment #0)
> So nsIPermissionManager should use the origin instead of using
> appId/isBrowser.
And nsIPermission as well.
Michael is working on moving the permission manager to use origins instead of host names, although we'd still need to use appId/isBrowser.  Is there any reason to not use those along side the origin?
Assignee: nobody → michael
Depends on: 1170200
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> Is there
> any reason to not use those along side the origin?
In Jonas' new security model on FirefoxOS, signed content needs to have different origin with unsigned content. So instead of adding another attribute check like signedContent, we try to fix up the usage of nsIPrincipal.appId/isBrowser first, make sure they all use nsIPrincipal.origin (now this will contain some extra information like !appId=12&inBrowser=true), so once security team has done the signature verification, they could update the principal with signing context.
Depends on: 1172022
(In reply to Yoshi Huang[:allstars.chh] from comment #3)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #2)
> > Is there
> > any reason to not use those along side the origin?
> In Jonas' new security model on FirefoxOS, signed content needs to have
> different origin with unsigned content. So instead of adding another
> attribute check like signedContent, we try to fix up the usage of
> nsIPrincipal.appId/isBrowser first, make sure they all use
> nsIPrincipal.origin (now this will contain some extra information like
> !appId=12&inBrowser=true), so once security team has done the signature
> verification, they could update the principal with signing context.

I see.  I think Michael is trying to use nsIPrincipal::GetOrigin here.  With that, once this issue is fixed, we would only need to worry about upgrade/downgrade handling of the newly added field to OriginAttributes.

One thing that we need to think about is whether an encoded string of a principal with only appId/inBrowserElement in it can be successfully decoded to a principal with a sensible value for the newly added OriginAttributes flags.

IOW, we need to ensure that adding new fields to OriginAttributes won't break the data saved in the permissions database.

Michael, can you please check that?  Thanks!
Flags: needinfo?(michael)
The mechanism for decoding nsIPrincipals from origin strings right now is landing (if I remember correctly) in bug 1155153 as OriginAttributes::PopulateFromSuffix. And being extended in bug 1169044 with OriginAttributes::PopulateFromOrigin.

Using the PopulateFromOrigin, you can get a origin without suffix, which can be NS_NewURI-ed, and a set of OriginAttributes which can be used to create a codebase principal. I believe that both PopulateFromSuffix, and by extension PopulateFromOrigin are able to handle the absence or presence of extra ones of these properties in the suffix string, which means that the permission manager wouldn't have to handle it.

In summary, I don't think it will be a problem if we use the standard mechanism for serializing & de-serializing origin strings.
Flags: needinfo?(michael)
The idea is that only non-default attributes are serialized, so upgrading should not be a problem (as long as it's always ok to upgrade a principal without attribute X to a principal with attribute X set to the default value).

Downgrading is a different story - I deliberately had baku make the code throw when trying to parse an unknown origin attributes, since otherwise Firefox N could merge two separate origins from Firefox N+1 into a single origin, which seems unsafe.

So it basically depends on whether the permission manager can handle a failure to parse an origin. I think dropping permissions whose origin fails to parse is the sensible thing to do.
If ehsan is okay with dropping permissions if we fail to parse an origin, I'll implement it like that.
Flags: needinfo?(ehsan)
Yes, that seems fine.
Flags: needinfo?(ehsan)
Depends on: 817007
Depends on: 1173523
This patch removes the test for mailto URIs in the permissions manager, as changes in the way that the permission manager works in the previous patch has broken mailto URI support in the permissions manager.

This change will require work on thunderbird's side to shift away from using the permission manager with mailto: URIs.
Attachment #8623696 - Flags: review?(ehsan)
I couldn't find a consumer for this function which I modified, so it may be a good idea to remove it entirely.
Attachment #8623701 - Flags: feedback?(ehsan)
Comment on attachment 8623696 [details] [diff] [review]
Part 2: Update unit tests for nsPermissionManager to reflect new internals

I completely forgot to update the matches and matchesURI tests (and their implementations) - so hold off on reviewing these patches until I fix that.
Attachment #8623696 - Flags: review?(ehsan)
Attachment #8623694 - Flags: review?(ehsan)
Insertions during migration should not be asynchronous >.>
Attachment #8623694 - Attachment is obsolete: true
Attachment #8623889 - Flags: review?(ehsan)
Changes to get reftests to have the correct permissions for hosted documents on various ports.
Attachment #8623698 - Attachment is obsolete: true
I think that with this change all tests pass again!
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d31cd8e5b67e
Attachment #8623696 - Attachment is obsolete: true
Attachment #8626793 - Flags: review?(ehsan)
Attachment #8626793 - Flags: review?(ehsan) → review+
Comment on attachment 8623701 [details] [diff] [review]
Part 7: Update automation.py.in to new moz_hosts schema

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

This code has been removed in bug 1177780.

But there is similar code in testing/mozbase/mozprofile/mozprofile/permissions.py and testing/mozbase/mozprofile/tests/permissions.py, which I think you should update.
Attachment #8623701 - Flags: feedback?(ehsan) → feedback-
Depends on: 1177780
I believe that those files (testing/mozbase/mozprofile/mozprofile/permissions.py and testing/mozbase/mozprofile/tests/permissions.py) are updated in Part 3: Update mozprofile to support new moz_hosts schema

It's good to know that permissions.py.in is not actually used anymore. I'll try to obsolete the part 7 patch (as an explanation for whatever spam appears after this in comments).
Attachment #8623701 - Attachment is obsolete: true
This is the most recent try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d31cd8e5b67e
I'm planning to push another one after I rebase, eliminating Part 7, and removing a temporary patch I have locally for a method I depend on from bug 1169044.
Attachment #8623699 - Flags: review?(jst)
Attachment #8623890 - Flags: review?(dbaron)
Attachment #8626627 - Flags: review?(ted)
Attachment #8623700 - Flags: review?(ehsan)
Blocks: 1172022
No longer depends on: 1172022
Comment on attachment 8623889 [details] [diff] [review]
Part 1: Update nsPermissionManager to use origins instead of hosts internally

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

I would appreciate if you can address the comments through an interdiff.  Thanks!

::: browser/app/permissions
@@ -1,1 @@
> -# This file has default permissions for the permission manager.

You should not change the line ending format of this file.

@@ +17,5 @@
> +origin	install	1	https://marketplace.firefox.com
> +
> +# Remote troubleshooting
> +origin	remote-troubleshooting	1	https://input.mozilla.org
> +origin	remote-troubleshooting	1	https://support.mozilla.org

Have you double checked the actual URLs that we use in the product for these permissions to make sure that the choice of https scheme is correct?

::: extensions/cookie/nsPermission.cpp
@@ +108,5 @@
> +  rv = ourURI->GetScheme(ourScheme);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (theirScheme != ourScheme) {
> +      return NS_OK;

Nit: 2 space indentation please, here and below.

::: extensions/cookie/nsPermissionManager.cpp
@@ +192,5 @@
>  };
>  
>  NS_IMPL_ISUPPORTS(AppClearDataObserver, nsIObserver)
>  
> +class UpgradeHostToOriginHelper {

Please mark this as MOZ_STACK_CLASS.

@@ +194,5 @@
>  NS_IMPL_ISUPPORTS(AppClearDataObserver, nsIObserver)
>  
> +class UpgradeHostToOriginHelper {
> +public:
> +  virtual void Insert(nsIPrincipal* aPrincipal, const nsAFlatCString& aType,

I don't think we can eat the return value here.  Like I have said elsewhere in the patch, you should at least warn when you detect an error...

@@ +199,5 @@
> +                      uint32_t aPermission, uint32_t aExpireType, int64_t aExpireTime,
> +                      int64_t aModificationTime) = 0;
> +};
> +
> +class UpgradeHostToOriginDBMigration : public UpgradeHostToOriginHelper {

Please mark the classes derived from UpgradeHostToOriginHelper as final.

@@ +204,5 @@
> +public:
> +  UpgradeHostToOriginDBMigration(mozIStorageConnection* aDBConn, int64_t* aID) : mDBConn(aDBConn)
> +                                                                               , mID(aID)
> +  {
> +    mDBConn->CreateAsyncStatement(NS_LITERAL_CSTRING(

Why are you creating an async statement?  Should it not be a sync one?

@@ +261,5 @@
> +    (*mID)++;
> +
> +    nsCOMPtr<mozIStoragePendingStatement> pending;
> +    rv = mInsertStmt->ExecuteAsync(nullptr, getter_AddRefs(pending));
> +    MOZ_ASSERT(NS_SUCCEEDED(rv));

You can't assert success here...

@@ +298,5 @@
> +  int64_t mID;
> +};
> +
> +nsresult
> +UpgradeHostToOriginAndInsert(const nsACString& aHost, const nsAFlatCString& aType,

Why do you need to accept nsAFlatCString?  That seems weird.  Can't you just use nsACString?

@@ +304,5 @@
> +                             int64_t aModificationTime, uint32_t aAppId, bool aIsInBrowserElement,
> +                             UpgradeHostToOriginHelper* aHelper)
> +{
> +  if (aHost.EqualsLiteral("<file>")) {
> +    // We no longer support the magic host <file>

Please add an NS_WARNING here...

@@ +313,5 @@
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult rv = NS_NewURI(getter_AddRefs(uri), aHost);
> +  if (NS_SUCCEEDED(rv)) {
> +    bool nullpScheme = false;
> +    if (NS_SUCCEEDED(uri->SchemeIs("moz-nullprincipal", &nullpScheme)) && nullpScheme) {

What are these URLs used for?  This deserves a comment at least.

@@ +326,5 @@
> +                    aExpireType, aExpireTime, aModificationTime);
> +    return NS_OK;
> +  }
> +
> +  // We always insert http:// and https:// URIs, so let's do that here

I think we want to do the opposite when upgrading: check in the places database first, and if there is no history entry for this, fall back to adding entries for both http and https.

@@ +350,5 @@
> +  // The user may use this host at non-standard ports or protocols, we can use their history
> +  // to guess what ports and protocols we want to add permissions for.
> +  // We find every URI which they have visited with this host (or a subdomain of this host),
> +  // and try to add it as a principal.
> +  nsCOMPtr<nsINavHistoryService> histSrv = do_GetService(NS_NAVHISTORYSERVICE_CONTRACTID);

Please ask :mak77 to review this part.

@@ +391,5 @@
> +
> +  rv = histResultContainer->SetContainerOpen(true);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  uint32_t childCount;

Nit: please initialize to 0.

@@ +400,5 @@
> +    nsCOMPtr<nsINavHistoryResultNode> child;
> +    histResultContainer->GetChild(i, getter_AddRefs(child));
> +    if (NS_FAILED(rv)) continue;
> +
> +    uint32_t type;

Nit: please initialize to 0.

@@ +818,5 @@
>  
> +    // Version 4->5 is the merging of host, appId, and isInBrowserElement into origin
> +    case 4:
> +      {
> +        bool tableExists;

Please initialize to false.

@@ +821,5 @@
> +      {
> +        bool tableExists;
> +        mDBConn->TableExists(NS_LITERAL_CSTRING("moz_hosts_new"), &tableExists);
> +        if (tableExists) {
> +          rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("DROP TABLE moz_hosts_new"));

Please add an NS_WARNING here.  This table should never exist!

@@ +845,5 @@
> +
> +        nsCOMPtr<mozIStoragePendingStatement> pending;
> +
> +        int64_t id = 0;
> +        nsAutoCString host, type, origin;

The variable origin is unused.  Please remove it.

@@ +852,5 @@
> +        int64_t expireTime;
> +        int64_t modificationTime;
> +        uint32_t appId;
> +        bool isInBrowserElement;
> +        bool hasResult;

Please initialize to false.

@@ +879,5 @@
> +          UpgradeHostToOriginAndInsert(host, type, permission,
> +                                       expireType, expireTime,
> +                                       modificationTime, appId,
> +                                       isInBrowserElement,
> +                                       &upHelper);

Please check the return value and at least warn if it fails.

@@ +882,5 @@
> +                                       isInBrowserElement,
> +                                       &upHelper);
> +        }
> +
> +        rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("DROP TABLE moz_hosts"));

Hmm, let's rename this to moz_hosts_old or some such, and remove it in a future upgrade, for the sake of disaster recovery on Nightly...

@@ +1639,5 @@
> +  if (NS_FAILED(rv)) {
> +    return nullptr;
> +  }
> +
> +  nsRefPtr<PermissionKey> key = new PermissionKey(origin);

You can construct the PermissionKey directly from the principal, right?

@@ +1860,5 @@
>  
> +    nsCOMPtr<nsIPrincipal> principal;
> +    GetPrincipalFromOrigin(entry->GetKey()->mOrigin, getter_AddRefs(principal));
> +
> +    uint32_t appId;

Nit: please initialize to NO_APP_ID.

@@ +1863,5 @@
> +
> +    uint32_t appId;
> +    principal->GetAppId(&appId);
> +
> +    bool isInBrowserElement;

Nit: please initialize to false.

@@ -1666,5 @@
> -  NS_ENSURE_SUCCESS(rv, rv);
> -
> -  nsCOMPtr<mozIStoragePendingStatement> pending;
> -  rv = removeStmt->ExecuteAsync(nullptr, getter_AddRefs(pending));
> -  NS_ENSURE_SUCCESS(rv, rv);

Do you know if this change is tested by anything?  It would be worth adding a new test for it if it's not covered already.

@@ +1925,5 @@
>    for (uint32_t i = 0; i < entry->GetPermissions().Length(); ++i) {
> +    nsCOMPtr<nsIPrincipal> principal;
> +    GetPrincipalFromOrigin(entry->GetKey()->mOrigin, getter_AddRefs(principal));
> +
> +    uint32_t appId;

Please initialize to NO_APP_ID.

@@ +2265,5 @@
> +      UpgradeHostToOriginHostfileImport upHelper(this, operation, id);
> +      UpgradeHostToOriginAndInsert(lineArray[3], lineArray[1], permission,
> +                                   nsIPermissionManager::EXPIRE_NEVER, 0,
> +                                   modificationTime, nsIScriptSecurityManager::NO_APP_ID,
> +                                   false, &upHelper);

Please check the return value.

@@ +2267,5 @@
> +                                   nsIPermissionManager::EXPIRE_NEVER, 0,
> +                                   modificationTime, nsIScriptSecurityManager::NO_APP_ID,
> +                                   false, &upHelper);
> +
> +    } else if (lineArray[0].EqualsLiteral(kMatchTypeOrigin) &&

Please extend test_permmanager_defaults.js to include tests for origin lines as well.

@@ +2268,5 @@
> +                                   modificationTime, nsIScriptSecurityManager::NO_APP_ID,
> +                                   false, &upHelper);
> +
> +    } else if (lineArray[0].EqualsLiteral(kMatchTypeOrigin) &&
> +        lineArray.Length() == 4) {

Nit: indentation.

@@ +2269,5 @@
> +                                   false, &upHelper);
> +
> +    } else if (lineArray[0].EqualsLiteral(kMatchTypeOrigin) &&
> +        lineArray.Length() == 4) {
> +      nsresult error;

Nit: please initialize to NS_OK.

::: extensions/cookie/nsPermissionManager.h
@@ +70,5 @@
>    class PermissionKey
>    {
>    public:
>      explicit PermissionKey(nsIPrincipal* aPrincipal);
> +    PermissionKey(const nsACString& aOrigin)

Mark this as explicit.
Attachment #8623889 - Flags: review?(ehsan) → review-
Attachment #8623700 - Flags: review?(ehsan) → review+
Updated to address review. The changes to tests will be uploaded in a seperate new patch for Part 2.
I'll upload the interdiff shortly.
Attachment #8623889 - Attachment is obsolete: true
Oops - messed up the comment in browser/app/permissions. Should be fixed now
Attachment #8627848 - Attachment is obsolete: true
Here's a diff of what has changed in my local copy of patch 1 since your last review. A couple of the issues you had with my patch (such as the async database statement, and the explicit constructor) were actually fixed in my local copy, and I had forgotten to push the patch to bugzilla, so they aren't in this diff (unfortunately).
Comment on attachment 8627849 [details] [diff] [review]
Part 2: Update unit tests for nsPermissionManager to reflect new internals

I'm asking for review again, because I changed the tests due to changes in Part 1.
Attachment #8627849 - Flags: review?(ehsan)
Attachment #8627856 - Flags: review?(ehsan)
Attachment #8627849 - Flags: review?(ehsan) → review+
Blocks: 812147
No longer depends on: 1163254
Blocks: 1179985
Attachment #8623699 - Flags: review?(jst) → review?(ehsan)
Comment on attachment 8623699 [details] [diff] [review]
Part 5: Update PermissionsUtils.importPrefBranch to use origins instead of hosts

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

You need to change all of the places where these prefs appear.  See <http://mxr.mozilla.org/mozilla-central/search?string=whitelist.add> for example.

Also, since these prefs might be set in existing user profiles, I think you also need to handle pref values that have a list of host names, and "upgrade" them to use origins.  For these prefs, I think it's enough to "upgrade" foo.com to https://foo.com.
Attachment #8623699 - Flags: review?(ehsan) → review-
Attachment #8627879 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 8627856 [details] [diff] [review]
Part 1: Update nsPermissionManager to use origins instead of hosts internally

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +903,5 @@
> +
> +        // We rename the old table to moz_hosts_v4 instead of dropping it, such that if
> +        // we discover that there was a problem with our migration code in the future, we have information
> +        // to roll-back with.
> +        rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("ALTER TABLE moz_hosts RENAME TO moz_hosts_v4"));

Can you please file a follow-up bug to remove this DB?
Attachment #8627856 - Flags: review?(ehsan) → review+
Updated the other users of whitelist.add and blacklist.add. The patch already contains code for upgrading legacy-style permissions preferences to http:// and https:// permissions
Attachment #8623699 - Attachment is obsolete: true
Attachment #8629528 - Flags: review?(ehsan)
Comment on attachment 8623890 [details] [diff] [review]
Part 4: Update reftest runners to support new moz_hosts schema

This is fine with me, although somebody who knows what these calls do should review it as well.
Attachment #8623890 - Flags: review?(dbaron) → review+
Attachment #8629528 - Flags: review?(ehsan) → review+
Tests for tracking protection have appeared and were failing because some tracking protection code depended on the old nsPermissionManager semantics. This changes the tracking manager to run on the new nsPermisisonManager.

This does have an impact on the semantics of tracking protection, but based on reading the comments on the tracking protection code I changed, the new semantics are actually what tracking protection wanted. Namely, previously tracking protection was only disabled based on host, and now the port is also taken into account (tracking protection code explicitly avoids using the scheme to determine whether to enable tracking protection).
Attachment #8630600 - Flags: review?(margaret.leibovic)
Comment on attachment 8630600 [details] [diff] [review]
Part 7: Update tracking protection to account for new nsPermissionManager semantics

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

This seems fine to me, but why isn't an equivalent desktop change required? Was that already handled elsewhere?
Attachment #8630600 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #38)
> Comment on attachment 8630600 [details] [diff] [review]
> Part 7: Update tracking protection to account for new nsPermissionManager
> semantics
> 
> Review of attachment 8630600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems fine to me, but why isn't an equivalent desktop change required?
> Was that already handled elsewhere?

When I looked over the desktop code, everything looked good (and tests pass), but I'm pretty sure that the changes in bug 1170200, part 2 to the tracking protection code were sufficient on desktop to have it work correctly with the new API, while on mobile more changes had to be made once the internal semantics changed.
Attachment #8626627 - Flags: review?(ted) → review?(ahalberstadt)
Comment on attachment 8626627 [details] [diff] [review]
Part 3: Update mozprofile to support new moz_hosts schema

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

Thanks! As stated on irc, this review is more for style than correctness (I know nothing about permission manager).

::: testing/mozbase/mozprofile/mozprofile/permissions.py
@@ +282,5 @@
> +                    cursor.execute(statement,
> +                                   (origin, perm, permission_type))
> +                else:
> +                    # The database is still using a legacy system based on hosts
> +                    # We can insert the poermission as a host

nit: permission

@@ +286,5 @@
> +                    # We can insert the poermission as a host
> +                    # XXX If this codepath is hit, migration will occur, and
> +                    # the tests won't be given permission to act correctly
> +                    # XXX We should probably remove support for old schemas,
> +                    # and instead nuke the database if we don't recognize it

Would you mind filing a follow-up and adding the bug number here?

::: testing/mozbase/mozprofile/tests/permissions.py
@@ +168,5 @@
>          entries = cur.fetchall()
>  
>          self.assertEqual(len(entries), 3)
>  
> +        columns = 7 if version == 5 else (9 if version == 4 else (8 if version == 3 else 6))

nit: expand the if statements or use a dict
Attachment #8626627 - Flags: review?(ahalberstadt) → review+
Blocks: 1058438
Blocks: 1180871
fixed patch is on inbound
Flags: needinfo?(michael)
Depends on: 1185239
Comment on attachment 8627856 [details] [diff] [review]
Part 1: Update nsPermissionManager to use origins instead of hosts internally


>diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp

>+nsresult
>+UpgradeHostToOriginAndInsert(const nsACString& aHost, const nsAFlatCString& aType,
>+                             uint32_t aPermission, uint32_t aExpireType, int64_t aExpireTime,
>+                             int64_t aModificationTime, uint32_t aAppId, bool aIsInBrowserElement,
>+                             UpgradeHostToOriginHelper* aHelper)
>+{

>+  // The user may use this host at non-standard ports or protocols, we can use their history
>+  // to guess what ports and protocols we want to add permissions for.
>+  // We find every URI which they have visited with this host (or a subdomain of this host),
>+  // and try to add it as a principal.
>+  nsCOMPtr<nsINavHistoryService> histSrv = do_GetService(NS_NAVHISTORYSERVICE_CONTRACTID);
>+
>+  nsCOMPtr<nsINavHistoryQuery> histQuery;
>+  rv = histSrv->GetNewQuery(getter_AddRefs(histQuery));

I don't think Android uses nsINavHistoryService and so we are crashes on every launch:

https://crash-stats.mozilla.com/report/index/589ff8da-4e63-43f3-9028-e6e222150718
I see bug 1185239 should cover Fennec too
Depends on: 1185332
Depends on: 1185340
Hey, The migration code is very poor. It causes browser hangup on start up when migrating.

This should be backed out, if you cannot fix the problem.
Flags: needinfo?(michael)
(In reply to Alice0775 White from comment #48)
> Hey, The migration code is very poor. It causes browser hangup on start up
> when migrating.
> 
> This should be backed out, if you cannot fix the problem.

There is a patch fixing this attached to bug 1185340. It should land later today.
Flags: needinfo?(michael)
Depends on: 1185613
Non-standard ports don't work properly with cookie permissions.  Existing permissions suddenly break when this patch lands because all ports used to be covered by the permission, and adding a new permission simply by specifying the host doesn't seem to detect usage of that port in the history.  The example I use is http://localhost:9091 which is in my history.  I tried adding the permission from the Firefox UI and through JS.  The only way I found to create a suitable permission was to explicitly specify http://localhost:9091 in the GUI (just localhost:9091 failed with a console error).
(In reply to Ian Nartowicz from comment #50)
> Non-standard ports don't work properly with cookie permissions.  Existing
> permissions suddenly break when this patch lands because all ports used to
> be covered by the permission, and adding a new permission simply by
> specifying the host doesn't seem to detect usage of that port in the
> history.  The example I use is http://localhost:9091 which is in my history.

We do migrate permission entries for the specific non-standard port numbers that you have in your history.  If that is not the case, that's a bug.  Please file it if that is what you are seeing.

> I tried adding the permission from the Firefox UI and through JS.  The only
> way I found to create a suitable permission was to explicitly specify
> http://localhost:9091 in the GUI (just localhost:9091 failed with a console
> error).

It would be hugely helpful if you could file a specific bug describing what part of the UI is showing the behavior that you are seeing so that we can fix it.  Thanks!
I raised bug 1189070 and bug 1189073.  Seems like the problems are mainly restricted to non-hostname URIs such as IP addresses and localhost.
Depends on: 1189070
Depends on: 1189073
No longer depends on: 1189073
Depends on: 1193200
Depends on: 1210379
See Also: → 1188348
QA Whiteboard: [COM=NSec]
Depends on: 1221974
Depends on: 1224210
Depends on: 1228246
Depends on: 1249889
Depends on: 1278054
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: