Closed Bug 1167100 Opened 5 years ago Closed 4 years ago

User nsIPrincipal.originAttribute in ContentPrincipalInfo

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- affected
firefox44 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 3 obsolete files)

As we will use originAttribute to wrap appId/inBrowser in Bug 1163254, so ContentPrincipalInfo should leverage that instead of handling appId/isBrowser by itself.
Component: IPC → DOM: Content Processes
No longer depends on: 1163254
Blocks: 1179985
Assignee: nobody → allstars.chh
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
(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.
Depends on: 1189235
(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.
Attached patch Patch. (obsolete) — Splinter Review
Attachment #8651732 - Flags: review?(bobbyholley)
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& ?
Attachment #8651732 - Flags: review?(bobbyholley) → review+
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
Status: NEW → ASSIGNED
Attached patch Patch v2. (obsolete) — Splinter Review
rebase
Attachment #8651732 - Attachment is obsolete: true
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
Attachment #8665230 - Flags: review?(bobbyholley)
Comment on attachment 8665230 [details] [diff] [review]
Patch v2.

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

Looks great, thanks Yoshi!
Attachment #8665230 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/d0e88c95f3c5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Depends on: 1209587
I had to back this out for causing crashes:
https://hg.mozilla.org/mozilla-central/rev/ccee6614fd9d
Status: RESOLVED → REOPENED
Flags: needinfo?(allstars.chh)
Resolution: FIXED → ---
Duplicate of this bug: 1209587
I think we should do bug 1209843 instead.
(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?
Attachment #8667635 - Attachment is obsolete: true
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.
Attached patch Patch. v3Splinter Review
rebase

https://hg.mozilla.org/try/pushloghtml?changeset=8483e1db11c1
Attachment #8665230 - Attachment is obsolete: true
Flags: needinfo?(allstars.chh)
https://hg.mozilla.org/mozilla-central/rev/27576be038ea
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 1211636
You need to log in before you can comment on or make changes to this bug.