Eliminate nsIPrincipal::jarPrefix

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: englehardt, Assigned: huseby)

Tracking

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [OA][domsecurity-backlog1])

Attachments

(1 attachment, 3 obsolete attachments)

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.
Depends on: 1165219, 1165277, 1165217
Assignee: nobody → senglehardt
Status: NEW → ASSIGNED
Posted patch 1189086.patch (obsolete) — Splinter Review
Sample patch. Will do additional testing once dependent bugs are fixed.
Yoshi, would you mind to let me know if I missed any dependent bugs?
Flags: needinfo?(allstars.chh)
Yeah, I think the bugs you listed are enough.
Flags: needinfo?(allstars.chh)
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.
Thanks Jan, added.
Depends on: 1195930
Component: Security: CAPS → DOM: Security
Priority: -- → P3
Whiteboard: [OA]
Whiteboard: [OA] → [OA][domsecurity-backlog1]
Assignee: bugzilla → nobody
Status: ASSIGNED → NEW
Assignee: nobody → huseby
the WIP patch covered everything except the ActorParent.cpp GetJarPrefix function.
Posted patch WIP patch (obsolete) — Splinter Review
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.
Attachment #8640760 - Attachment is obsolete: true
(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.
Jan,

Do you have a plan for removing it from ActorsParent.cpp?
Flags: needinfo?(jvarga)
updated patch to revert the removal of the function from ActorsParent.cpp.  Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5fa6014db2b145456d4a30c32c4c19653d33d26
updated the patch
Attachment #8799544 - Attachment is obsolete: true
Flags: needinfo?(jvarga)
Attachment #8799578 - Flags: review?(jvarga)
(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 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.
Attachment #8799578 - Flags: review?(jvarga)
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
Attachment #8799578 - Flags: review?(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.
Attachment #8799578 - Flags: review?(dveditz) → review+
(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.
Flags: needinfo?(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.
Flags: needinfo?(dveditz)
The try push looks good.
already r+, rebased. try push looks good.
Attachment #8799578 - Attachment is obsolete: true
Attachment #8804041 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f19085b4d55b
Eliminate nsIPrincipal::jarPrefix. r=dveditz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f19085b4d55b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.