1167100 - User nsIPrincipal.originAttribute in ContentPrincipalInfo

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

As we will use originAttribute to wrap appId/inBrowser in Bug 1163254, so ContentPrincipalInfo should leverage that instead of handling appId/isBrowser by itself.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

So far I think following changes are needed:
1. Bug 1188776 and Bug 1165272 need to be done first.
2. dom/base/nsJSEnvironment.cpp for updating structured clone of Principal.
3. dom/cache/CacheStorage.cpp for checking appId is unknown.
4. dom/cache/DBSchema.cpp for using OriginAttributes and constructing ContentPrincipalInfo.
5. dom/workers/ServiceWorkerRegistrar.cpp for reading/writing originAttributes into serviceworker.txt
and test case in dom/workers/test/gtest/TestReadWrite.cpp.
6. updating ipc/glue/BackgroundUtils.cpp and ipc/glue/PBackgroundSharedTypes.ipdlh

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Yoshi Huang[:allstars.chh] from comment #1)
> 4. dom/cache/DBSchema.cpp for using OriginAttributes and constructing
> ContentPrincipalInfo.

The serialized origin attributes are stored in the database.  If this changes we need a migration written to avoid corrupting the data.

> 5. dom/workers/ServiceWorkerRegistrar.cpp for reading/writing
> originAttributes into serviceworker.txt
> and test case in dom/workers/test/gtest/TestReadWrite.cpp.

Same thing.  This needs to migrate existing data on disk.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> (In reply to Yoshi Huang[:allstars.chh] from comment #1)
> > 4. dom/cache/DBSchema.cpp for using OriginAttributes and constructing
> > ContentPrincipalInfo.
> 
> The serialized origin attributes are stored in the database.  If this
> changes we need a migration written to avoid corrupting the data.
> 
I won't change the serialized origin.
I will only change how we get OriginAttributes in InsertEntry
and how we construct ContentPrincipalInfo in ReadResponse.

Just change the const
> > 5. dom/workers/ServiceWorkerRegistrar.cpp for reading/writing
> > originAttributes into serviceworker.txt
> > and test case in dom/workers/test/gtest/TestReadWrite.cpp.
> 
> Same thing.  This needs to migrate existing data on disk.
Dimi will fix this in Bug 1189235.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8651732 [details] [diff] [review]
Patch.

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

This looks great - thank you Yoshi!

::: ipc/glue/BackgroundUtils.cpp
@@ +80,3 @@
>          rv = secMan->GetSimpleCodebasePrincipal(uri, getter_AddRefs(principal));
>        } else {
> +        principal = BasePrincipal::CreateCodebasePrincipal(uri, const_cast<OriginAttributes&>(info.attrs()));

instead of the const_cast, can you just make CreateCodebasePrincipal accept const OriginAttributes& ?

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I found the Mn-e10s will always have UNEXPECTED_ERROR after applying my patch on try server, although my local run is green.
Will spend more time to check this.

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&tochange=487c57e377c2&fromchange=dbd6e7b9a9e1

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch v2.

rebase

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8665230 [details] [diff] [review]
Patch v2.

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

re-send r? again for Bug 1203916 and Bug 1163254 have been landed.

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

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8665230 [details] [diff] [review]
Patch v2.

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

Looks great, thanks Yoshi!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/d0e88c95f3c5

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for causing crashes:
https://hg.mozilla.org/mozilla-central/rev/ccee6614fd9d

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: fix UNKNOWN_APP_ID error

Comment 15

[Bobby Holley (:bholley)]

I think we should do bug 1209843 instead.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Wes Kocher (:KWierso) from comment #12)
> I had to back this out for causing crashes:
> https://hg.mozilla.org/mozilla-central/rev/ccee6614fd9d

Could this be the reason for crashes as reported in bug 1209508?

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8665230 [details] [diff] [review]
Patch v2.

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

::: ipc/glue/BackgroundUtils.cpp
@@ +202,5 @@
>    bool isUnknownAppId;
>    rv = aPrincipal->GetUnknownAppId(&isUnknownAppId);
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }

line 202 ~ line 206 are dead code,
will remove this as well.

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v3

rebase

https://hg.mozilla.org/try/pushloghtml?changeset=8483e1db11c1

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/27576be038ea

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27576be038ea