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.

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.

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

(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.

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

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 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 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.

(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?

(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.

(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).

(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.

(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.

(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?

(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.

(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).

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).

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.

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.

(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

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.

Ben, no problem! I haven't had a chance to return to this anyway. Makes sense to me, marking as duplicate of 1112071.