Switch permission manager initialization to be async

RESOLVED FIXED in Firefox 55

Status

()

Core
Permission Manager
RESOLVED FIXED
7 months ago
a month ago

People

(Reporter: Ehsan, Assigned: mystor)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(12 attachments, 6 obsolete attachments)

11.44 KB, patch
qdot
: review+
Details | Diff | Splinter Review
2.40 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
2.41 KB, patch
qdot
: review+
Details | Diff | Splinter Review
3.31 KB, patch
kitcambridge
: review+
Details | Diff | Splinter Review
3.60 KB, patch
Details | Diff | Splinter Review
18.32 KB, patch
Details | Diff | Splinter Review
10.83 KB, patch
Details | Diff | Splinter Review
1.89 KB, patch
Details | Diff | Splinter Review
3.94 KB, patch
Details | Diff | Splinter Review
778 bytes, patch
billm
: review+
Details | Diff | Splinter Review
5.25 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.96 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
Permission manager initialization can take a long time.

<https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-02-05&keys=PContent%253A%253AMsg_ReadPermissions&max_channel_version=nightly%252F54&measure=IPC_SYNC_LATENCY_MS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2017-02-02&table=0&trim=1&use_submission_date=0>

Start	End	IPC_SYNC_LATENCY_MS Count
0	1	0 (0%)
1	3	125.67k (41.87%)
3	8	44.81k (14.93%)
8	20	30.4k (10.13%)
20	50	27.56k (9.18%)
50	126	26.3k (8.76%)
126	317	24.91k (8.3%)
317	796	12.91k (4.3%)
796	2k	5.73k (1.91%)
2k	Infinity	1.83k (0.61%)

The other consumer of Msg_ReadPermissions is nsIPermissionManager::RefreshPermision() which is dead code being removed in bug 1337054.
This is our 4th worst sync IPC per https://docs.google.com/spreadsheets/d/1x_BWVlnQPg0DHbsrvPFX7g89lnFGa3lAIHWD_pLa_dE/edit#gid=738048355.

Andrew can you please find an owner for this one?
Flags: needinfo?(overholt)
Michael said he'd take a look.
Assignee: nobody → michael
Flags: needinfo?(overholt)
See Also: → bug 1313683
See Also: bug 1313683
Duplicate of this bug: 1313683
I have a suggestion for fixing this: let's do something similar to the plan of bug 1331680, aka start the child process without the knowledge about any permissions, and only send down the permissions for the sites that you visit, in a way very similar to the proposal there (but for any LOAD_DOCUMENT_URI).  This has the additional privacy benefit that a rogue content process won't be able to query arbitrary permissions or modify them, only the ones for the websites loaded in that process.

For this to make sense we'd need to audit the TestPermission call sites.  I think in the content process we can return UNKNOWN_ACTION if the parent hasn't sent down the permission, and in case that does the wrong thing in a call site we find in the audit we can discuss further.

What do you think Michael?
(In reply to :Ehsan Akhgari from comment #4)
> I have a suggestion for fixing this: let's do something similar to the plan
> of bug 1331680, aka start the child process without the knowledge about any
> permissions, and only send down the permissions for the sites that you
> visit, in a way very similar to the proposal there (but for any
> LOAD_DOCUMENT_URI).  This has the additional privacy benefit that a rogue
> content process won't be able to query arbitrary permissions or modify them,
> only the ones for the websites loaded in that process.
> 
> For this to make sense we'd need to audit the TestPermission call sites.  I
> think in the content process we can return UNKNOWN_ACTION if the parent
> hasn't sent down the permission, and in case that does the wrong thing in a
> call site we find in the audit we can discuss further.
> 
> What do you think Michael?

That sounds like a neat idea. Unfortunately, we currently provide an API for enumerating all permissions (nsIPermissionManager.enumerator), and that would produce slightly odd different results under this model. I'm not completely sure if we care.

I quickly threw together a patch today which simply pushes all of the permissions to the child process as efficiently as possible and without blocking any event loops - We can use something like it as a fallback if this patch doesn't work out. It avoids a bunch of computation work and thread blocking compared to the current implementation, so it should be an improvement whether or not this works out. I'm worried about its worst case situation, however, as I have seen permissions databases which have tens of thousands of entries due to some add-on or another.

Would you like me to take over bug 1331680, as it seems like a subset of the implementation work required for your ideas here (especially because it only has one consumer)?
Flags: needinfo?(ehsan)
(In reply to Michael Layzell [:mystor] from comment #5)
> (In reply to :Ehsan Akhgari from comment #4)
> > I have a suggestion for fixing this: let's do something similar to the plan
> > of bug 1331680, aka start the child process without the knowledge about any
> > permissions, and only send down the permissions for the sites that you
> > visit, in a way very similar to the proposal there (but for any
> > LOAD_DOCUMENT_URI).  This has the additional privacy benefit that a rogue
> > content process won't be able to query arbitrary permissions or modify them,
> > only the ones for the websites loaded in that process.
> > 
> > For this to make sense we'd need to audit the TestPermission call sites.  I
> > think in the content process we can return UNKNOWN_ACTION if the parent
> > hasn't sent down the permission, and in case that does the wrong thing in a
> > call site we find in the audit we can discuss further.
> > 
> > What do you think Michael?
> 
> That sounds like a neat idea. Unfortunately, we currently provide an API for
> enumerating all permissions (nsIPermissionManager.enumerator), and that
> would produce slightly odd different results under this model. I'm not
> completely sure if we care.

Yeah I think that's OK.  But this is another thing to watch for in the audit.  I think it's OK if some call sites need to be modified to do the right thing post this change.

> I quickly threw together a patch today which simply pushes all of the
> permissions to the child process as efficiently as possible and without
> blocking any event loops - We can use something like it as a fallback if
> this patch doesn't work out. It avoids a bunch of computation work and
> thread blocking compared to the current implementation, so it should be an
> improvement whether or not this works out. I'm worried about its worst case
> situation, however, as I have seen permissions databases which have tens of
> thousands of entries due to some add-on or another.

Yeah the permission manager DB can be quite large.  However in the worst case we should be able to send the data from parent to child asynchronously after the child starts, but we have to be careful to deal with the possibility that the permission manager may need the data before it has arrived.  With my suggestion this problem sort of fixes itself.

> Would you like me to take over bug 1331680, as it seems like a subset of the
> implementation work required for your ideas here (especially because it only
> has one consumer)?

No, I think Amy is going to work on that bug.  I don't think we can really share code since the cookie stuff runs off of the PCookieService protocol under PNecko but here we probably want to use PContent directly, etc.  And the logic doesn't match between these two bugs in any way other than the abstract idea.  :-)  So I think these can be done independently in parallel.
Flags: needinfo?(ehsan)
Blocks: 1287730
Created attachment 8844086 [details] [diff] [review]
Part 1: Add a mechanism for grouping permissions into groups to be sent over IPC

This is a first pass implementation of asynchronous permission communication to the content process. It handles the primary protocols which permissions can be found on (http[s]:// and ftp://).

I plan to add support for file:// in a follow up (which is more complex due to the protocol not performing IPC to load the content).

MozReview-Commit-ID: IQiSsVGaAOQ
Attachment #8844086 - Flags: review?(ehsan)
Created attachment 8844087 [details] [diff] [review]
Part 2: Replace the synchronous ReadPermissions API with async APIs

These APIs are intended to use the mechanism defined in Part 1.

Part 3 implements the usage of these APIs to synchronize permissions.

MozReview-Commit-ID: HNKyDPtoaHl
Attachment #8844087 - Flags: review?(ehsan)
Created attachment 8844088 [details] [diff] [review]
Part 3: Send down http[s] and ftp permissions as they are needed. Send down other permissions at startup

MozReview-Commit-ID: CUKPvFp6zpF
Attachment #8844088 - Flags: review?(ehsan)
Created attachment 8844089 [details] [diff] [review]
Part 4: Assert that the ipcKey for a principal is avaliable when creating a PermissionKey in the child process

MozReview-Commit-ID: G9TynCKgCVF
Attachment #8844089 - Flags: review?(ehsan)
Created attachment 8844090 [details] [diff] [review]
Part 5: Add support for finer-grained permission update messages to ContentParent

MozReview-Commit-ID: 6y6eBLWPTue
Attachment #8844090 - Flags: review?(ehsan)
Created attachment 8844091 [details] [diff] [review]
Part 6: Disable nsPermissionManager::GetEnumerator in the content process

This property allows enumerating over all permissions. As the content
process no longer has access to all permissions, this method cannot be
correctly implemented in the content process anymore.

Because of that, we now error with NS_ERROR_NOT_AVALIABLE when it is
accessed in the content process.

MozReview-Commit-ID: BLNeYYcZhIi
Attachment #8844091 - Flags: review?(kit)
Created attachment 8844092 [details] [diff] [review]
Part 7: Refactor nsObjectLoadingContent::GetTypeOfContent logic out into nsContentUtils

MozReview-Commit-ID: IJQNhQZzx3y
Attachment #8844092 - Flags: review?(kyle)
Created attachment 8844093 [details] [diff] [review]
Part 8: Add nsIRequest::LOAD_HTML_OBJECT_DATA flag to identify requests loaded by nsObjectLoadingContent

MozReview-Commit-ID: 3hgLRCeuiyD
Attachment #8844093 - Flags: review?(mcmanus)
Created attachment 8844094 [details] [diff] [review]
Part 9: Check LOAD_HTML_OBJECT_DATA in ContentParent::TransmitPermsFor

MozReview-Commit-ID: 5MBAUe5zbvU
Attachment #8844094 - Flags: review?(ehsan)
Doing a try push to make sure the assertions aren't firing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdf3a75d725e996c81072ec9f2389d7c54532c45
Comment on attachment 8844086 [details] [diff] [review]
Part 1: Add a mechanism for grouping permissions into groups to be sent over IPC

Moving a bunch of reviews from ehsan->baku as we've talked about this plan too much together and it would be good to get another set of eyes on the idea.
Attachment #8844086 - Flags: review?(ehsan) → review?(amarchesini)
Attachment #8844087 - Flags: review?(ehsan) → review?(amarchesini)
Attachment #8844088 - Flags: review?(ehsan) → review?(amarchesini)
Attachment #8844089 - Flags: review?(ehsan) → review?(amarchesini)
Attachment #8844090 - Flags: review?(ehsan) → review?(amarchesini)
Attachment #8844094 - Flags: review?(ehsan) → review?(kyle)
Comment on attachment 8844091 [details] [diff] [review]
Part 6: Disable nsPermissionManager::GetEnumerator in the content process

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

Looking closer, I think you uncovered another bug in `NotificationTelemetryService::Init`. It should only enumerate permissions and add a permission observer in the parent at startup. IIUC, though, `Init` will run when we get the service for the first time in the content process, meaning it'll add another observer and double-count permission changes.

I know this was already broken, but would you mind changing `Init` to bail early if it's in the content process, and asserting that `AddPermissionChangeObserver` and `RecordPermissions` are only ever called in the parent?

Thanks for catching this!
Attachment #8844091 - Flags: review?(kit)
Created attachment 8844111 [details] [diff] [review]
Part 6: Disable nsPermissionManager::GetEnumerator in the content process

This property allows enumerating over all permissions. As the content
process no longer has access to all permissions, this method cannot be
correctly implemented in the content process anymore.

Because of that, we now error with NS_ERROR_NOT_AVALIABLE when it is
accessed in the content process.

MozReview-Commit-ID: BLNeYYcZhIi
Attachment #8844111 - Flags: review?(kit)
Attachment #8844091 - Attachment is obsolete: true
Comment on attachment 8844111 [details] [diff] [review]
Part 6: Disable nsPermissionManager::GetEnumerator in the content process

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

Thanks!
Attachment #8844111 - Flags: review?(kit) → review+
Comment on attachment 8844088 [details] [diff] [review]
Part 3: Send down http[s] and ftp permissions as they are needed. Send down other permissions at startup

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

::: dom/ipc/ContentParent.cpp
@@ +4982,5 @@
>    ProcessHangMonitor::ForcePaint(mHangMonitorActor, aTabParent, aLayerObserverEpoch);
>  }
>  
> +nsresult
> +ContentParent::TransmitPermsFor(nsIChannel* aChannel)

Permissions instead Perms? In all our code base we have 'Permissions'.

@@ +4983,5 @@
>  }
>  
> +nsresult
> +ContentParent::TransmitPermsFor(nsIChannel* aChannel)
> +{

MOZ_ASSERT(aChannel);

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +461,5 @@
>  
> +  // Send down any permissions which are relevant to this URL if we are
> +  // performing a document load.
> +  PContentParent* pcp = Manager()->Manager();
> +  static_cast<ContentParent*>(pcp)->TransmitPermsFor(chan);

this can fail.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1120,5 @@
>  
> +  // Send down any permissions which are relevant to this URL if we are
> +  // performing a document load.
> +  PContentParent* pcp = Manager()->Manager();
> +  static_cast<ContentParent*>(pcp)->TransmitPermsFor(chan);

this can fail.

::: netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp
@@ +324,5 @@
> +  // Send down any permissions which are relevant to this URL if we are
> +  // performing a document load.
> +  PContentParent* pcp = Manager()->Manager();
> +  static_cast<ContentParent*>(pcp)->TransmitPermsFor(chan);
> +

this can fail.
Attachment #8844088 - Flags: review?(amarchesini) → review+
Comment on attachment 8844087 [details] [diff] [review]
Part 2: Replace the synchronous ReadPermissions API with async APIs

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

I don't like ipcKey. Can you describe better what it is in the nsIPermissionManager.idl and change its name?

::: dom/ipc/PContent.ipdl
@@ +696,5 @@
>      async Deactivate(PBrowser aTab);
>  
>      async ParentActivated(PBrowser aTab, bool aActivated);
>  
> +    async SetPermissionsWithKey(nsCString aIpcKey, Permission[] aPermissions);

aPermissionKey ?

::: extensions/cookie/nsPermissionManager.cpp
@@ +2892,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsPermissionManager::GetPermissionsWithKey(const nsACString& aIpcKey,

aPermissionKey ? aIPCKey is too generic.

@@ +2916,5 @@
> +    }
> +
> +    // Get the IPC Key and make sure that it matches the aIpcKey passed in.
> +    nsAutoCString ipcKey;
> +    GetKeyForPrincipal(principal, ipcKey);

here you should deal with an error.
Attachment #8844087 - Flags: review?(amarchesini) → review+
Comment on attachment 8844086 [details] [diff] [review]
Part 1: Add a mechanism for grouping permissions into groups to be sent over IPC

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

Propagate the errors.

::: extensions/cookie/nsPermissionManager.cpp
@@ +2936,5 @@
> +
> +// XXX: Support file URIs here as well!
> +/* static */ void
> +nsPermissionManager::GetKeyForPrincipal(nsIPrincipal* aPrincipal, nsACString& aKey)
> +{

MOZ_ASSERT(aPrincipal);

@@ +2942,5 @@
> +
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult rv = aPrincipal->GetURI(getter_AddRefs(uri));
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return;

why no error propagation?
Attachment #8844086 - Flags: review?(amarchesini) → review+
Comment on attachment 8844090 [details] [diff] [review]
Part 5: Add support for finer-grained permission update messages to ContentParent

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

::: extensions/cookie/nsPermissionManager.cpp
@@ +1567,5 @@
>      IPC::Permission permission(origin, aType, aPermission,
>                                 aExpireType, aExpireTime);
>  
> +    nsAutoCString ipcKey;
> +    GetKeyForPrincipal(aPrincipal, ipcKey);

This could fail... see comments in previous reviews.
Attachment #8844090 - Flags: review?(amarchesini) → review+
Attachment #8844089 - Flags: review?(amarchesini) → review+
Created attachment 8845029 [details] [diff] [review]
Part 1: Add a mechanism for grouping permissions into groups to be sent over IPC

I have made the name changes (No longer called IPC key) that you requested. I also added comments explaining the reason why GetKeyForPrincipal is infallible. 

MozReview-Commit-ID: IQiSsVGaAOQ
Attachment #8844086 - Attachment is obsolete: true
Attachment #8844087 - Attachment is obsolete: true
Attachment #8844088 - Attachment is obsolete: true
Attachment #8844089 - Attachment is obsolete: true
Attachment #8844090 - Attachment is obsolete: true
Created attachment 8845030 [details] [diff] [review]
Part 2: Replace the synchronous ReadPermissions API with async APIs

These APIs are intended to use the mechanism defined in Part 1.

Part 3 implements the usage of these APIs to synchronize permissions.

MozReview-Commit-ID: HNKyDPtoaHl
Created attachment 8845031 [details] [diff] [review]
Part 3: Send down http[s] and ftp permissions as they are needed. Send down other permissions at startup

MozReview-Commit-ID: CUKPvFp6zpF
Created attachment 8845032 [details] [diff] [review]
Part 4: Assert that the ipcKey for a principal is avaliable when creating a PermissionKey in the child process

MozReview-Commit-ID: G9TynCKgCVF
Created attachment 8845033 [details] [diff] [review]
Part 5: Add support for finer-grained permission update messages to ContentParent

MozReview-Commit-ID: 6y6eBLWPTue
You asked me to NI? you once the new patches went up :)
Flags: needinfo?(amarchesini)
Blocks: 1345573
Attachment #8844093 - Flags: review?(mcmanus) → review+
Attachment #8844092 - Flags: review?(kyle) → review+
Attachment #8844094 - Flags: review?(kyle) → review+

Updated

6 months ago
Whiteboard: [qf:p1]
Created attachment 8847163 [details] [diff] [review]
Part 10: Remove ReadPermissions from the sync-messages list

I was just getting ready to finally actually land this and realized I hadn't removed the call from the sync IPC list.

MozReview-Commit-ID: 8Ki4KEHKnQm
Attachment #8847163 - Flags: review?(wmccloskey)

Updated

5 months ago
Attachment #8847163 - Flags: review?(wmccloskey) → review+

Comment 32

5 months ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7568383eb18d
Part 1: Add a mechanism for grouping permissions into groups to be sent over IPC, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2d603dbdaf
Part 2: Replace the synchronous ReadPermissions API with async APIs, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9674c8a9d4f
Part 3: Send down http[s] and ftp permissions as they are needed. Send down other permissions at startup, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6e7570df3e
Part 4: Assert that the ipcKey for a principal is avaliable when creating a PermissionKey in the child process, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4ee0af3ff0
Part 5: Add support for finer-grained permission update messages to ContentParent, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/33447647d4e8
Part 6: Disable nsPermissionManager::GetEnumerator in the content process, r=kitcambridge
https://hg.mozilla.org/integration/mozilla-inbound/rev/cca9db8d2256
Part 7: Refactor nsObjectLoadingContent::GetTypeOfContent logic out into nsContentUtils, r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/40b8605b5e48
Part 8: Add nsIRequest::LOAD_HTML_OBJECT_DATA flag to identify requests loaded by nsObjectLoadingContent, r=mcmanus
https://hg.mozilla.org/integration/mozilla-inbound/rev/31a996094ef4
Part 9: Check LOAD_HTML_OBJECT_DATA in ContentParent::TransmitPermsFor, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/0690f70aa69a
Part 10: Remove ReadPermissions from the sync-messages list, r=billm
Backed out bug 1337056 and bug 1345573 for crashing xpcshell test test_bug930456_child.js on debug:

bug 1337056:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0c83461506c421e5c7473cce9a53d1bfee06d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddcfb1e70d7230c491dc0a2cb756320de28eea17
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd3ecc8e83179b40e50e39645b9efa7e53fda7d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/36c22158aa94be8bbe82e970462a36b157f775e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b5387adabe9cabf299ca25ea22a42df00511ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/95a8674a048d7ce09de02b9340e075b321bcb68a
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc53ad27cd41785974bc83cf2d015a7cf1bee33a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b2c6dacc63afc23dcaffe377a9e4b1df2d1b30
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b93025f476945108ac6988ad00a61f3c54f331b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5fb7e5eb46b2083a6c424307c961bd255458f83

bug 1345573:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8767bffeadca0b8d907eb251149965745ecd8b85
https://hg.mozilla.org/integration/mozilla-inbound/rev/30bb3bc160d3e6022892c717c00d114695546a07

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f5f05a774023c7e9cc8eedbfbc91ba117f49274a&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=83785901&repo=mozilla-inbound

12:53:31  WARNING -  PROCESS-CRASH | toolkit/components/search/tests/xpcshell/test_bug930456_child.js | application crashed [@ mozalloc_abort(char const*)]
12:53:31     INFO -  Crash dump filename: /var/folders/64/m7jhlc3x3b32lz348yvd80pc00000w/T/xpc-other-OWG_6b/0B714EBA-12B1-4D08-B04E-DE790AF0F45F.dmp
12:53:31     INFO -  Operating system: Mac OS X
12:53:31     INFO -                    10.10.5 14F27
12:53:31     INFO -  CPU: amd64
12:53:31     INFO -       family 6 model 69 stepping 1
12:53:31     INFO -       4 CPUs
12:53:31     INFO -  GPU: UNKNOWN
12:53:31     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
12:53:31     INFO -  Crash address: 0x0
12:53:31     INFO -  Process uptime: 6 seconds
12:53:31     INFO -  Thread 0 (crashed)
12:53:31     INFO -   0  libmozglue.dylib!mozalloc_abort(char const*) [mozalloc_abort.cpp:f5f05a774023 : 33 + 0x0]
12:53:31     INFO -      rax = 0x0000000000000000   rdx = 0x00007fff7c5691f8
12:53:31     INFO -      rcx = 0x0000000000000000   rbx = 0x00007fff7c569c50
12:53:31     INFO -      rsi = 0x00002b0000002b00   rdi = 0x00002a0000002b03
12:53:31     INFO -      rbp = 0x00007fff534e4c20   rsp = 0x00007fff534e4c10
12:53:31     INFO -       r8 = 0x00007fff534e4bc0    r9 = 0x00007fff7c4f4300
12:53:31     INFO -      r10 = 0x0000000000000000   r11 = 0x0000000000000246
12:53:31     INFO -      r12 = 0x0000000000000001   r13 = 0x0000000111872568
12:53:31     INFO -      r14 = 0x00007fff534e4c90   r15 = 0x00007fff534e6e8b
12:53:31     INFO -      rip = 0x0000000113032439
12:53:31     INFO -      Found by: given as instruction pointer in context
12:53:31     INFO -   1  XUL!Abort(char const*) [nsDebugImpl.cpp:f5f05a774023 : 441 + 0x5]
12:53:31     INFO -      rbx = 0x00007fff7c569c50   rbp = 0x00007fff534e4c30
12:53:31     INFO -      rsp = 0x00007fff534e4c30   r12 = 0x0000000000000001
12:53:31     INFO -      r13 = 0x0000000111872568   r14 = 0x00007fff534e4c90
12:53:31     INFO -      r15 = 0x00007fff534e6e8b   rip = 0x000000010ca3d7a9
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -   2  XUL!NS_DebugBreak [nsDebugImpl.cpp:f5f05a774023 : 428 + 0x8]
12:53:31     INFO -      rbx = 0x00007fff7c569c50   rbp = 0x00007fff534e50c0
12:53:31     INFO -      rsp = 0x00007fff534e4c40   r12 = 0x0000000000000001
12:53:31     INFO -      r13 = 0x0000000111872568   r14 = 0x00007fff534e4c90
12:53:31     INFO -      r15 = 0x00007fff534e6e8b   rip = 0x000000010ca3d590
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -   3  XUL!nsPermissionManager::PermissionKey::CreateFromPrincipal(nsIPrincipal*, nsresult&) [nsPermissionManager.cpp:f5f05a774023 : 661 + 0x1e]
12:53:31     INFO -      rbx = 0x00007fff534e50d8   rbp = 0x00007fff534e51f0
12:53:31     INFO -      rsp = 0x00007fff534e50d0   r12 = 0x00007fff534e5168
12:53:31     INFO -      r13 = 0x0000000000000000   r14 = 0x0000000119482920
12:53:31     INFO -      r15 = 0x00007fff534e5128   rip = 0x000000010d5ecc76
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -   4  XUL!nsPermissionManager::AddInternal(nsIPrincipal*, nsCString const&, unsigned int, long long, unsigned int, long long, long long, nsPermissionManager::NotifyOperationType, nsPermissionManager::DBOperationType, bool) [nsPermissionManager.cpp:f5f05a774023 : 1638 + 0xc]
12:53:31     INFO -      rbx = 0x0000000119482920   rbp = 0x00007fff534e5390
12:53:31     INFO -      rsp = 0x00007fff534e5200   r12 = 0x0000000113362980
12:53:31     INFO -      r13 = 0x0000000000000000   r14 = 0x00007fff534e5300
12:53:31     INFO -      r15 = 0x0000000000000000   rip = 0x000000010d5f4aff
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -   5  XUL!mozilla::dom::ContentChild::RecvAddPermission(IPC::Permission const&) [ContentChild.cpp:f5f05a774023 : 2234 + 0x25]
12:53:31     INFO -      rbx = 0x0000000119482920   rbp = 0x00007fff534e54b0
12:53:31     INFO -      rsp = 0x00007fff534e53a0   r12 = 0x00007fff534e55a0
12:53:31     INFO -      r13 = 0x00007fff534e5420   r14 = 0x0000000113362980
12:53:31     INFO -      r15 = 0x00007fff534e53f8   rip = 0x000000010f1d396c
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -   6  XUL!mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) [PContentChild.cpp:f5f05a774023 : 6832 + 0x13]
12:53:31     INFO -      rbx = 0x00007fff534e56b0   rbp = 0x00007fff534e5700
12:53:31     INFO -      rsp = 0x00007fff534e54c0   r12 = 0x0000000113393bc8
12:53:31     INFO -      r13 = 0x00007fff534e55b0   r14 = 0x00000001133ac020
12:53:31     INFO -      r15 = 0x0000000113393bd0   rip = 0x000000010d40eba9
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -   7  XUL!mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) [MessageChannel.cpp:f5f05a774023 : 1872 + 0x9]
12:53:31     INFO -      rbx = 0x0000000000000001   rbp = 0x00007fff534e5760
12:53:31     INFO -      rsp = 0x00007fff534e5710   r12 = 0x0000000000000000
12:53:31     INFO -      r13 = 0x00000001133ac118   r14 = 0x0000000113393bc8
12:53:31     INFO -      r15 = 0x00000000ffffff00   rip = 0x000000010d0893cb
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -   8  XUL!mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) [MessageChannel.cpp:f5f05a774023 : 1807 + 0xb]
12:53:31     INFO -      rbx = 0x0000000113389330   rbp = 0x00007fff534e5820
12:53:31     INFO -      rsp = 0x00007fff534e5770   r12 = 0x0000000113393bc8
12:53:31     INFO -      r13 = 0x0000000113306e70   r14 = 0x00000001133ac118
12:53:31     INFO -      r15 = 0x00000000ffffffff   rip = 0x000000010d087197
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -   9  XUL!mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) [MessageChannel.cpp:f5f05a774023 : 1680 + 0xb]
12:53:31     INFO -      rbx = 0x0000000113393b00   rbp = 0x00007fff534e5880
12:53:31     INFO -      rsp = 0x00007fff534e5830   r12 = 0x0000000113393b70
12:53:31     INFO -      r13 = 0x0000000113306e70   r14 = 0x00000001133ac118
12:53:31     INFO -      r15 = 0x0000000113393bc8   rip = 0x000000010d0883a2
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  10  XUL!mozilla::ipc::MessageChannel::MessageTask::Run() [MessageChannel.cpp:f5f05a774023 : 1713 + 0x8]
12:53:31     INFO -      rbx = 0x0000000113393b70   rbp = 0x00007fff534e58a0
12:53:31     INFO -      rsp = 0x00007fff534e5890   r12 = 0x00007fff534e5918
12:53:31     INFO -      r13 = 0x0000000113306e70   r14 = 0x0000000113389330
12:53:31     INFO -      r15 = 0x00000001133c8040   rip = 0x000000010d088b48
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  11  XUL!nsThread::ProcessNextEvent(bool, bool*) [nsThread.cpp:f5f05a774023 : 1269 + 0x6]
12:53:31     INFO -      rbx = 0x00007fff534e5900   rbp = 0x00007fff534e59a0
12:53:31     INFO -      rsp = 0x00007fff534e58b0   r12 = 0x00007fff534e5918
12:53:31     INFO -      r13 = 0x0000000113306e70   r14 = 0x00000001133c8090
12:53:31     INFO -      r15 = 0x00000001133c8040   rip = 0x000000010caf939c
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  12  XUL!NS_ProcessNextEvent(nsIThread*, bool) [nsThreadUtils.cpp:f5f05a774023 : 389 + 0xd]
12:53:31     INFO -      rbx = 0x0000000000000000   rbp = 0x00007fff534e59d0
12:53:31     INFO -      rsp = 0x00007fff534e59b0   r12 = 0x0000000000000001
12:53:31     INFO -      r13 = 0x0000000113306e70   r14 = 0x00007fff534e5bd8
12:53:31     INFO -      r15 = 0x0000000000000001   rip = 0x000000010caf798e
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  13  XUL!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [MessagePump.cpp:f5f05a774023 : 96 + 0xb]
12:53:31     INFO -      rbx = 0x0000000113306e90   rbp = 0x00007fff534e5a40
12:53:31     INFO -      rsp = 0x00007fff534e59e0   r12 = 0x0000000000000001
12:53:31     INFO -      r13 = 0x0000000113306e70   r14 = 0x00007fff534e5bd8
12:53:31     INFO -      r15 = 0x0000000000000001   rip = 0x000000010d08c833
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  14  XUL!MessageLoop::Run() [message_loop.cc:f5f05a774023 : 231 + 0x5]
12:53:31     INFO -      rbx = 0x000000011961a640   rbp = 0x00007fff534e5a70
12:53:31     INFO -      rsp = 0x00007fff534e5a50   r12 = 0x0000000000000003
12:53:31     INFO -      r13 = 0x00007fff534e5e40   r14 = 0x00000000534e5b01
12:53:31     INFO -      r15 = 0x00007fff534e5bd8   rip = 0x000000010d040efc
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  15  XUL!XRE_RunAppShell() [nsEmbedFunctions.cpp:f5f05a774023 : 866 + 0x8]
12:53:31     INFO -      rbx = 0x000000011961a640   rbp = 0x00007fff534e5ac0
12:53:31     INFO -      rsp = 0x00007fff534e5a80   r12 = 0x0000000000000003
12:53:31     INFO -      r13 = 0x00007fff534e5e40   r14 = 0x00000000534e5b01
12:53:31     INFO -      r15 = 0x00007fff534e5bd8   rip = 0x0000000110a63683
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  16  XUL!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) [MessagePump.cpp:f5f05a774023 : 269 + 0x5]
12:53:31     INFO -      rbx = 0x00007fff534e5bd8   rbp = 0x00007fff534e5af0
12:53:31     INFO -      rsp = 0x00007fff534e5ad0   r12 = 0x0000000000000003
12:53:31     INFO -      r13 = 0x00007fff534e5e40   r14 = 0x0000000113306e70
12:53:31     INFO -      r15 = 0x0000000080004005   rip = 0x000000010d08d282
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  17  XUL!MessageLoop::Run() [message_loop.cc:f5f05a774023 : 231 + 0x5]
12:53:31     INFO -      rbx = 0x0000000000000001   rbp = 0x00007fff534e5b20
12:53:31     INFO -      rsp = 0x00007fff534e5b00   r12 = 0x0000000000000003
12:53:31     INFO -      r13 = 0x00007fff534e5e40   r14 = 0x0000000000000012
12:53:31     INFO -      r15 = 0x0000000080004005   rip = 0x000000010d040efc
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  18  XUL!XRE_InitChildProcess(int, char**, XREChildData const*) [nsEmbedFunctions.cpp:f5f05a774023 : 695 + 0x5]
12:53:31     INFO -      rbx = 0x0000000000000001   rbp = 0x00007fff534e5de0
12:53:31     INFO -      rsp = 0x00007fff534e5b30   r12 = 0x0000000000000003
12:53:31     INFO -      r13 = 0x00007fff534e5e40   r14 = 0x0000000000000012
12:53:31     INFO -      r15 = 0x0000000080004005   rip = 0x0000000110a63297
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  19  plugin-container!main [plugin-container.cpp:f5f05a774023 : 64 + 0x13]
12:53:31     INFO -      rbx = 0x0000000113334188   rbp = 0x00007fff534e5e20
12:53:31     INFO -      rsp = 0x00007fff534e5df0   r12 = 0x0000000000000000
12:53:31     INFO -      r13 = 0x0000000000000000   r14 = 0x00007fff534e5e40
12:53:31     INFO -      r15 = 0x0000000000000015   rip = 0x000000010c719ee9
12:53:31     INFO -      Found by: call frame info
12:53:31     INFO -  20  plugin-container!start + 0x34
12:53:31     INFO -      rbx = 0x0000000000000000   rbp = 0x00007fff534e5e30
12:53:31     INFO -      rsp = 0x00007fff534e5e30   r12 = 0x0000000000000000
12:53:31     INFO -      r13 = 0x0000000000000000   r14 = 0x0000000000000000
12:53:31     INFO -      r15 = 0x0000000000000000   rip = 0x000000010c719e84
12:53:31     INFO -      Found by: call frame info
Flags: needinfo?(michael)
Created attachment 8848632 [details] [diff] [review]
Part 12: Send down permissions when calling createAboutBlankContentViewer

Ugh. Every time I do another try push thinking it's the last one I find new problems which were hidden before.

I hope this is the last part :).

MozReview-Commit-ID: LigZnHM34CC
Attachment #8848632 - Flags: review?(amarchesini)
Created attachment 8848633 [details] [diff] [review]
Part 11: Change the permission key assertion to a fatal assert on debug builds

MozReview-Commit-ID: HTxvlomRKWy
Attachment #8848633 - Flags: review?(ehsan)
Attachment #8848633 - Flags: review?(ehsan) → review+
Attachment #8848632 - Flags: review?(amarchesini) → review+

Comment 36

5 months ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6d2ebc113f
Part 1: Add a mechanism for grouping permissions into groups to be sent over IPC, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/215ca1b54ff1
Part 2: Replace the synchronous ReadPermissions API with async APIs, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b8ead5376b
Part 3: Send down http[s] and ftp permissions as they are needed. Send down other permissions at startup, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc5c70cb1511
Part 4: Assert that the ipcKey for a principal is avaliable when creating a PermissionKey in the child process, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/a21d709fd0b0
Part 5: Add support for finer-grained permission update messages to ContentParent, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01b69497592
Part 6: Disable nsPermissionManager::GetEnumerator in the content process, r=kitcambridge
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc03c82cb565
Part 7: Refactor nsObjectLoadingContent::GetTypeOfContent logic out into nsContentUtils, r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c41abe5360d
Part 8: Add nsIRequest::LOAD_HTML_OBJECT_DATA flag to identify requests loaded by nsObjectLoadingContent, r=mcmanus
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea56abef5ea
Part 9: Check LOAD_HTML_OBJECT_DATA in ContentParent::TransmitPermsFor, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/55fe81687988
Part 10: Remove ReadPermissions from the sync-messages list, r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/2686c6ee997d
Part 11: Change the permission key assertion to a fatal assert on debug builds, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/f86ef08fdae0
Part 12: Send down permissions when calling createAboutBlankContentViewer, r=baku
Flags: needinfo?(michael)
Flags: needinfo?(amarchesini)

Comment 37

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff6d2ebc113f
https://hg.mozilla.org/mozilla-central/rev/215ca1b54ff1
https://hg.mozilla.org/mozilla-central/rev/c2b8ead5376b
https://hg.mozilla.org/mozilla-central/rev/dc5c70cb1511
https://hg.mozilla.org/mozilla-central/rev/a21d709fd0b0
https://hg.mozilla.org/mozilla-central/rev/d01b69497592
https://hg.mozilla.org/mozilla-central/rev/cc03c82cb565
https://hg.mozilla.org/mozilla-central/rev/8c41abe5360d
https://hg.mozilla.org/mozilla-central/rev/8ea56abef5ea
https://hg.mozilla.org/mozilla-central/rev/55fe81687988
https://hg.mozilla.org/mozilla-central/rev/2686c6ee997d
https://hg.mozilla.org/mozilla-central/rev/f86ef08fdae0
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 38

5 months ago
Is this crash(bp-d67b8fd2-3107-4e34-875a-eeaa72170322) related to these changes?
Depends on: 1349634
(In reply to Toshihiro Yamada from comment #38)
> Is this crash(bp-d67b8fd2-3107-4e34-875a-eeaa72170322) related to these
> changes?

Yeah, filed bug 1349634.
Depends on: 1349922
No longer depends on: 1349922
Depends on: 1350252
No longer depends on: 1350252

Updated

5 months ago
Depends on: 1352772
Depends on: 1353179
Blocks: 1379345
You need to log in before you can comment on or make changes to this bug.