1165219 - Use origin in ManagerId

Status: RESOLVED DUPLICATE of bug 1112071

(Core :: DOM: Service Workers, defect)

Description

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

From Bug 940273 we imeplemened cache for service worker, now for the new security model we will update nsIPrincipal in Bug 1163254.
I found that ManagerId uses origin/appId/isBrowser as index, we should use the new origin attributes in nsIPrincipal for this.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I believe Jan already has some patches in flight to adjust this bit of code.  You probably want to talk with him to avoid conflicting.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Also, the ManagerId must be usable off the main thread.  I believe nsIPrincipal is main thread only.  How do you propose to solve this?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Also, the ManagerId must be usable off the main thread.  I believe
> nsIPrincipal is main thread only.  How do you propose to solve this?

As discussed in the dependent bug, the idea is that nsIPrincipal exposes a stringified representation of the data we need.

Comment 4

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

seems some work is already landed in Bug 1130775.
maybe we should considering removing jarPrefix and use 'origin' instead of extendedOrigin in this bug a

Comment 5

[Steven Englehardt [:englehardt]]

Attachment: 1165219.patch

Since the previous implementation was just concatenating jarPrefix and originNoSuffix, I've replaced it with origin. I've also renamed ExtendedOrigin -> Origin to reflect this. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ef5eb140ca1

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8640748 [details] [diff] [review]
1165219.patch

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

Looks right, but someone who knows the code should also review.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8640748 [details] [diff] [review]
1165219.patch

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

This is undo'ing specific review feedback I gave Jan in the other bug.  I really, really dislike that we call this thing "origin", but its not the same origin that is used in the specs.  I think its confusing for new contributors.

If you're making the entire tree consistent in the use of the term "origin" to mean this composite thing, though, I guess I'll capitulate.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Ben Kelly [:bkelly] from comment #7)
> This is undo'ing specific review feedback I gave Jan in the other bug.  I
> really, really dislike that we call this thing "origin", but its not the
> same origin that is used in the specs.  I think its confusing for new
> contributors.

The issue is that we have additional attributes that need to be taken into account any time a spec says "if the origin of A is equal to the origin of B". If we called it something else, people would be less likely to use it (and would use the old thing, which has been called nsIPrincipal::origin since the dawn of time), and then we'd have security bugs (just like if we, say, didn't include the port or the protocol).

Do you have a different suggestion of how it should be done?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Bobby Holley (:bholley) from comment #8)
> The issue is that we have additional attributes that need to be taken into
> account any time a spec says "if the origin of A is equal to the origin of
> B". If we called it something else, people would be less likely to use it
> (and would use the old thing, which has been called nsIPrincipal::origin
> since the dawn of time), and then we'd have security bugs (just like if we,
> say, didn't include the port or the protocol).

Yea, but then people expect to be able to use it as essentially a base URL (like you can in the specs IIUC) and it no longer works.  Like the persona bustage on b2g right now.

> Do you have a different suggestion of how it should be done?

Nothing practical.  I think I'm coming to the conclusion that gecko-specific app extensions like this are not a good idea.

But I already r+'d the patch.  If you're willing to fix all the "origin" uses to be correct and document it well, then I relent.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Ben Kelly [:bkelly] from comment #9)
> Yea, but then people expect to be able to use it as essentially a base URL
> (like you can in the specs IIUC) and it no longer works.  Like the persona
> bustage on b2g right now.

You already can't do that, because origin isn't always a meaningful URI (consider nonce principals and the like).
 
> Nothing practical.  I think I'm coming to the conclusion that gecko-specific
> app extensions like this are not a good idea.

There is a complexity cost, sure - but there are a lot of important outcomes (notably Firefox OS itself) that we couldn't achieve without it.
 
> But I already r+'d the patch.  If you're willing to fix all the "origin"
> uses to be correct and document it well, then I relent.

FWIW, I'm not personally doing that (I'm guiding the design and mentoring implementors in my space cycles).

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Bobby Holley (:bholley) from comment #10)
> (In reply to Ben Kelly [:bkelly] from comment #9)
> > Yea, but then people expect to be able to use it as essentially a base URL
> > (like you can in the specs IIUC) and it no longer works.  Like the persona
> > bustage on b2g right now.
> 
> You already can't do that, because origin isn't always a meaningful URI
> (consider nonce principals and the like).

Right.  People do checks for content principal and then do it.

> > Nothing practical.  I think I'm coming to the conclusion that gecko-specific
> > app extensions like this are not a good idea.
> 
> There is a complexity cost, sure - but there are a lot of important outcomes
> (notably Firefox OS itself) that we couldn't achieve without it.

People outside mozilla want some kind of signing capability for service workers.  I'd rather see us pursue that avenue than roto-till our tree for a gecko-specific extension.  I didn't raise this during the fxos architecture planning, though, so I'm not trying to block things.  I just don't think the current plan helps the web much.

Comment 12

[Bobby Holley (:bholley)]

(In reply to Ben Kelly [:bkelly] from comment #11)

> Right.  People do checks for content principal and then do it.

You mean content principal _and_ not null principal, right? This means people already need to know about a lot of special stuff, and already need to think before exposing nsIPrincipal::origin to the web.

> People outside mozilla want some kind of signing capability for service
> workers.

That's only the signedPkg use case though, right? We still need to support appId, inBrowser for the time being (which currently need to be manually appended to these strings with :t:f: nastiness), as well as all the things that Desktop is doing (userContextId, addonId, etc).

> I'd rather see us pursue that avenue than roto-till our tree for a
> gecko-specific extension.

The old approach required roto-tilling our tree each time somebody had a dimension that they need to add to origins. This happened once with private browsing, again with appId/inBrowser, and was about to happen a third and fourth time "user contexts" and the new e10s addon stuff. The chances of getting each one of those things right are basically nil, which is why we need a generalized mechanism that all consumers will consider by default.

> I didn't raise this during the fxos architecture planning, though, so I'm not trying to block things. 

Sure, but I'm trying to convince you anyway. :-)

> I just don't think the current plan helps the web much.

These are internal extensions, so they don't visibly help the web, much like Compartments don't visibly help the web. But they provide an abstraction that lets us do powerful things with our products with O(1) work, which make our products better, which helps us advance our mission. It's all trade-offs, and I think the benefits here pretty clearly outweigh the costs.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Bobby Holley (:bholley) from comment #12)
> The old approach required roto-tilling our tree each time somebody had a
> dimension that they need to add to origins. This happened once with private
> browsing, again with appId/inBrowser, and was about to happen a third and
> fourth time "user contexts" and the new e10s addon stuff. The chances of
> getting each one of those things right are basically nil, which is why we
> need a generalized mechanism that all consumers will consider by default.

Ok. I didn't realize this was needed for things other than b2g apps.  I still rather it was called something other than "origin", but whatever.

I hope you have a static_assert() somewhere reminding people if they add a new "dimension" they have to find everywhere one of these strings has been serialized to disk and write a migration.  Or will the string always support old formats?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Ben Kelly [:bkelly] from comment #13)
> I hope you have a static_assert() somewhere reminding people if they add a
> new "dimension" they have to find everywhere one of these strings has been
> serialized to disk and write a migration.  Or will the string always support
> old formats?

That's the idea. The serialization only includes &foo=bar if |bar| is a "non-default" value of |foo|. So long as it's always correct to assume that old serialized data contains the "default" value for any newly-added OriginAttributes, no migration is needed.

So we just need to make sure that all serializations use .origin or .originSuffix as appropriate, and then everything should Just Work.

Comment 15

[Steven Englehardt [:englehardt]]

(In reply to Ben Kelly [:bkelly] from comment #7)
> Comment on attachment 8640748 [details] [diff] [review]
> 1165219.patch
> 
> Review of attachment 8640748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is undo'ing specific review feedback I gave Jan in the other bug.  I
> really, really dislike that we call this thing "origin", but its not the
> same origin that is used in the specs.  I think its confusing for new
> contributors.
> 
> If you're making the entire tree consistent in the use of the term "origin"
> to mean this composite thing, though, I guess I'll capitulate.

I just wanted to follow up to address the naming concern: nsIPrincipal now refers to the composite string as 'origin': https://dxr.mozilla.org/mozilla-central/source/caps/nsIPrincipal.idl#184. For consistency I think 'origin' makes the most sense for this bug. The other bugs making these changes (see Bug 1179985) are following the same format as well (e.g. Bug 1165277).

Comment 16

[Steven Englehardt [:englehardt]]

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

Comment 17

[Steven Englehardt [:englehardt]]

Yoshi, reading this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1182285#c39 makes me think it's safe to ignore the B2G ICS Emulator tests as well? Mochitest-3 is timing out with this patch applied (https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ef5eb140ca1).

Comment 18

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

you may check the first two if they are caused by your patch

118 INFO TEST-UNEXPECTED-FAIL | dom/cache/test/mochitest/test_cache.html | Test timed out.
143 INFO TEST-UNEXPECTED-FAIL | dom/cache/test/mochitest/test_cache_add.html | Test timed out.

others should be fine.

Comment 19

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

if the failure only happens on emulator your could skip it first and file another bug for fixing it.

Add skip-if = toolkit == "gonk" for your tests.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Yoshi Huang[:allstars.chh] from comment #19)
> if the failure only happens on emulator your could skip it first and file
> another bug for fixing it.
> 
> Add skip-if = toolkit == "gonk" for your tests.

I'm sorry, but I don't think this is ok for these changes.  b2g is the only place that actually exercises the appId/browserFlag parts of the origin attributes.

Also, the try log indicates that a SEGV occurred:

18:42:51 ERROR - 07-31 01:41:36.371 F/libc ( 768): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1)
18:42:51 ERROR - This usually indicates the B2G process has crashed

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Note, I have found I actually need to use QuotaManager::GetInfoForPrincipal() in ManagerId to get the QM's modified origin.  I think that will make these changes unnecessary.  See bug 1112071.  Sorry!

If you're ok with that, then we can probably more this a dupe of bug 1112071 or as WONTFIX.

Comment 22

[Steven Englehardt [:englehardt]]

Ben, no problem! I haven't had a chance to return to this anyway.

Makes sense to me, marking as duplicate of 1112071.