1189086 - Eliminate nsIPrincipal::jarPrefix

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Steven Englehardt [:englehardt]]

jarPrefix is currently a concatenation of appId and inMozBrowser as cookieJar was removed in Bug 1182347. Bug 1179985 seeks to replace this with origin/originSuffix. Once the remaining consumers (Bug 1165219, Bug 1165277, and Bug 1165217) are migrated to originSuffix, jarPrefix can be removed.

Comment 1

[Steven Englehardt [:englehardt]]

Attachment: 1189086.patch

Sample patch. Will do additional testing once dependent bugs are fixed.

Comment 2

[Steven Englehardt [:englehardt]]

Yoshi, would you mind to let me know if I missed any dependent bugs?

Comment 3

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

Yeah, I think the bugs you listed are enough.

Comment 4

[Jan Varga [:janv]]

Bug 1165217 is just about fixing nsIUsageCallback. Quota manager uses jarPrefix + originNoSuffix to find storage directories (for example: <profile>/storage/persistent/1007+f+app+++system.gaiamobile.org). So we need to fix it/convert it somehow before jarPrefix can be removed.

Comment 5

[Steven Englehardt [:englehardt]]

Thanks Jan, added.

Comment 6

[Dave Huseby [:huseby]]

I rebased and built, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a2927ae61782be1ec1bae55dd56aa841271051c

Comment 7

[Dave Huseby [:huseby]]

the WIP patch covered everything except the ActorParent.cpp GetJarPrefix function.

Comment 8

[Dave Huseby [:huseby]]

rebased, fixed build issues, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f883ae8a26b966dd75d3367a21f902d3cbc204db

Comment 9

[Dave Huseby [:huseby]]

Attachment: WIP patch

this is the updated WIP rebased and fixed.  it completely removes the GetJarPrefix code.  The only thing that is of concern to me are the changes I made CreateDirectoryMetadata to remove the calls to GetJarPrefix.  I assume that the suffix created from the origin attributes should be sufficient since it includes the origin attributes that were being fed into GetJarPrefix.  I'm just not sure what changing the metadata string will do to stored state.

Comment 10

[Jan Varga [:janv]]

(In reply to Dave Huseby [:huseby] from comment #9)
> Created attachment 8799544 [details] [diff] [review]
> WIP patch
> 
> this is the updated WIP rebased and fixed.  it completely removes the
> GetJarPrefix code.  The only thing that is of concern to me are the changes
> I made CreateDirectoryMetadata to remove the calls to GetJarPrefix.  I
> assume that the suffix created from the origin attributes should be
> sufficient since it includes the origin attributes that were being fed into
> GetJarPrefix.  I'm just not sure what changing the metadata string will do
> to stored state.

Please don't remove it from ActorsParent.cpp
The code there handles reading/upgrading of .metadata (version 1) files in user profile which were created in an older FF.
I forked the GetJarPrefix() method because I knew it will be removed at some point.

Comment 11

[Dave Huseby [:huseby]]

Jan,

Do you have a plan for removing it from ActorsParent.cpp?

Comment 12

[Dave Huseby [:huseby]]

updated patch to revert the removal of the function from ActorsParent.cpp.  Try:

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

Comment 13

[Dave Huseby [:huseby]]

Attachment: Removes the jar prefix code that is no longer needed.

updated the patch

Comment 14

[Jan Varga [:janv]]

(In reply to Dave Huseby [:huseby] from comment #11)
> Jan,
> 
> Do you have a plan for removing it from ActorsParent.cpp?

Why would I want to remove it ?
Those .metadata files were created in an older FF and we need to read/upgrade them in newer FF.
The old format didn't use origin attributes, it used origin and group prefixed with a jar prefix.
The code is self contained, it's not exported anywhere, I don't see why we need a plan to remove it.

Comment 15

[Jan Varga [:janv]]

Comment on attachment 8799578 [details] [diff] [review]
Removes the jar prefix code that is no longer needed.

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

I'm a reviewer for dom/quota, that's why I commented on ActorsParent.cpp changes, but I'm not a reviewer for dom/caps.
You need to find some one else, sorry about that.

Comment 16

[Dave Huseby [:huseby]]

Comment on attachment 8799578 [details] [diff] [review]
Removes the jar prefix code that is no longer needed.

Hi Dan,

According to the modules list, you're a peer for DOM/Security/Caps which this patch is a part of.  Would you mind reviewing it for me?

Thanks,
--dave

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 8799578 [details] [diff] [review]
Removes the jar prefix code that is no longer needed.

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

r=dveditz

I worry about changing the binary compatibility of nsIPrincipal and nsIScriptSecurityManager because in the past external apps hooked into those and we caused crashes. Maybe that doesn't happen as much anymore, but we should keep an eye out for hints of it when we hit Beta.

Comment 18

[Dave Huseby [:huseby]]

(In reply to Daniel Veditz [:dveditz] from comment #17)
> we should keep an eye out for hints of it when we hit Beta.

Can you elaborate on what that looks like? I'd like to better understand what you mean by that.

Comment 19

[Dave Huseby [:huseby]]

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

Comment 20

[Daniel Veditz [:dveditz]]

(In reply to Dave Huseby [:huseby] from comment #18)
> (In reply to Daniel Veditz [:dveditz] from comment #17)
> > we should keep an eye out for hints of it when we hit Beta.
> 
> Can you elaborate on what that looks like? I'd like to better understand
> what you mean by that.

New crashes, possibly in 3rd party .dll's, possibly in principal or ScriptSecurityManager code. It /should/ be OK.

Comment 21

[Dave Huseby [:huseby]]

The try push looks good.

Comment 22

[Dave Huseby [:huseby]]

Attachment: rebased patch r+ dveditz

already r+, rebased. try push looks good.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f19085b4d55b
Eliminate nsIPrincipal::jarPrefix. r=dveditz

Comment 24

[Phil Ringnalda (:philor)]

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