1297446 - simple addons trigger large amounts of sync IPC in e10s

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Please see sample range [122250, 122599] on the content process in this profile:

https://cleopatra.io/#report=03ca7a5d631e37ea3d638d62f6c06a0b32a52727

There is a ton if sync IPC happening.

This is coming from RemoteAddonChild.jsm here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsChild.jsm#417

Which is being triggered from "document-element-inserted" being observed in the addon:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/addoncompat/RemoteAddonsParent.jsm#384

The addon in question here is the GeckoProfiler itself.  AFAICT its triggering this simply by doing:

  request('sdk/panel')

The panel module pulls in panel/events:

https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/panel.js#30

Which then observes "document-element-inserted":

https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/panel/events.js#25

Basically anything using sdk/panel is going to trigger this sync IPC.

Blake, what do you think?

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This is basically bug 1167802.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Ok.  I think its a problem if our main profiler addon interferes with perf to this degree, though.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, I agree. We can mark the profiler as multiprocessCompatible. That should eliminate the shims. Benoit, are you still the maintainer? You just need to add a key to install.rdf and make sure everything still works.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #1)
> This is basically bug 1167802.

This is odd. This should have been fixed by bug 1196975. system/events.js is already using shim waivers.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Ben Kelly [:bkelly] from comment #0)
> Please see sample range [122250, 122599] on the content process in this
> profile:
> 
> https://cleopatra.io/#report=03ca7a5d631e37ea3d638d62f6c06a0b32a52727

For some reason I only see necko/predictor activity on that section...

> 
> There is a ton if sync IPC happening.
> 
> This is coming from RemoteAddonChild.jsm here:
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> addoncompat/RemoteAddonsChild.jsm#417
> 
> Which is being triggered from "document-element-inserted" being observed in
> the addon:
> 
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/
> addoncompat/RemoteAddonsParent.jsm#384
> 
> The addon in question here is the GeckoProfiler itself.  AFAICT its
> triggering this simply by doing:
> 
>   request('sdk/panel')
> 
> The panel module pulls in panel/events:
> 
> https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/
> panel.js#30
> 
> Which then observes "document-element-inserted":
> 
> https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/
> panel/events.js#25
> 
> Basically anything using sdk/panel is going to trigger this sync IPC.
> 
> Blake, what do you think?

Are you sure about this diagnostic? Panel's should use system/events.js which is working hard to avoid
the shim layer.

There is a really handy flag I tend to use to hunt down these kind of issues: dom.ipc.shims.enabledWarnings (it should add some warnings to the console when the shim layer is hit)

I only see the shim being used from here: http://searchfox.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/l10n-html.js#118

(which should be investigated...)

Is there something I'm missing here?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Screenshot 2016-08-24 09.21.04.png

Pretty sure.  This is what I see in cleopatra.

There is network predictor stuff causing problems, but thats in the parent process and its later in the trace.  I filed bug 1295565 for that.

Comment 7

[Benoit Girard (:BenWa)]

Does JPM not support it? Do I have to repack the addon and add it manually?

Comment 8

[Markus Stange [:mstange]]

Maybe we just need to repack the addon with a more recent version of the addon-sdk.

Comment 9

[Benoit Girard (:BenWa)]

I updated the addon to 1.16.18 which should include the most recent addon-sdk.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think it's the multiprocess permission:
https://developer.mozilla.org/en-US/Add-ons/SDK/Tools/package_json#permissions
Might be good to check the install.rdf file that gets generated though.

Comment 11

[Gabor Krizsanits (INACTIVE)]

(In reply to Benoit Girard (:BenWa) from comment #9)
> I updated the addon to 1.16.18 which should include the most recent
> addon-sdk.

Yeah the old version seem to include the old SDK in the xpi (which did not use shim waiving), so that might have been the reason for the problem. But it's a bit odd that I don't see the warnings on the console only for l10n-html so maybe the included SDK is ignored entirely. Do you have any reason not to strip the SDK from your xpi btw? (I thought we do that automatically so I don't quite get why is it included at all)

(In reply to Ben Kelly [:bkelly] from comment #6)
> Pretty sure.  This is what I see in cleopatra.

Thanks I see it too now. I get that we hit the RemoteAddonsChild part, I was just wondering if it's the result of the panel code. Can you explain to me where do you see it from that this is the result of the panel code and not the l10n code? (given that the old SDK is included into the xpi I can imagine that it is the case I just want to be able to reproduce it so I can fix it)

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> Thanks I see it too now. I get that we hit the RemoteAddonsChild part, I was
> just wondering if it's the result of the panel code. Can you explain to me
> where do you see it from that this is the result of the panel code and not
> the l10n code? (given that the old SDK is included into the xpi I can
> imagine that it is the case I just want to be able to reproduce it so I can
> fix it)

See:

http://logs.glob.uno/?c=mozilla%23gfx&s=15+Aug+2016&e=15+Aug+2016&h=GeckoProfiler#c278617

We don't know for sure its sdk/panel, but it is coming from something in GeckoProfiler.  GeckoProfiler does not directly observe document-element-inserted itself AFAICT.  The sdk/panel seemed the most likely after looking at the modules in use.  But again, not 100% certain.

Comment 13

[Gabor Krizsanits (INACTIVE)]

Attachment: Shimwaiver for l10n. v1

(In reply to Ben Kelly [:bkelly] from comment #12)
> We don't know for sure its sdk/panel, but it is coming from something in
> GeckoProfiler.  GeckoProfiler does not directly observe
> document-element-inserted itself AFAICT.  The sdk/panel seemed the most
> likely after looking at the modules in use.  But again, not 100% certain.

If it's from the panel then repacking should fix it but I don't think so, I believe that the SDK that is shipped with Firefox is being used and the one in the xpi is ignored. Which means it should waive the shim layer entirely for panels. If it's from l10n (what I've seen triggering the shim layer for the profiler and does attach a listener to document-element-inserted) this patch should fix it. This is affecting a lot of SDK add-ons if not all of them, so we should land this either way. Let's see if it fixes the profiler or not.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8785230 [details] [diff] [review]
Shimwaiver for l10n. v1

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

OK. It looks like Jetpack's localization is supposed to work for content scripts, so this should be fine. Thanks Gabor. Hopefully this fixes it.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Is this something we can or should uplift to other branches?

Comment 16

[Gabor Krizsanits (INACTIVE)]

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

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't see why not. It's a very straightforward patch. Would you mind testing to see if it works, Ben? It looks like Gabor has some try builds there.

Comment 18

[Gabor Krizsanits (INACTIVE)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a32c1e67f222930df2fc7f0b1806c365af3187b
Bug 1297446 - Shimwaiver for l10n. r=billm

Comment 19

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8785230 [details] [diff] [review]
Shimwaiver for l10n. v1

I think we want this for aurora but I'm afraid we're too late for beta... but let's try. For aurora it is I think more important because there might be some add-on testing on the beta channel in the next cycle for add-on white listing and this patch affects SDK add-ons.

For the release it's less important since e10s is turned off for add-on users there, however since it affects the profiler add-on it would be really nice for internal work...

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: SDK based add-ons trigger the shim layer and can cause performance issues unnecessary (janks in the content process)
[Describe test coverage new/current, TreeHerder]: looks green on try, should be OK on inbound too soon
[Risks and why]: it's a very simple patch I don't see any risk
[String/UUID change made/needed]: none

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Looks like the large blocks of sync IPC for the addon are gone.

The sync IPC I see left are:

* A large block of sync IPC during initial tab load for PContent::SendReadPermissions()
* A large block of sync IPC from content-sessionStore.js
* A single blip for PScreenManager's GetPrimaryScreen()

Those two large blocks happened when clicking on a placeholder tab that caused session store to reload it.

Anyway, the issue originally reported here seems to be fixed.  Thanks!

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

For reference, the profile I was looking at in comment 20 is:

https://cleopatra.io/#report=c1bd0f092a1b8c00eda13426faa4413412df7ded

This is an optimized DEBUG build with this bug's patch applied.  I had to do a local build since I needed symbols.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a32c1e67f22

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

This is very late in beta to uplift but let's try it. We are planning on enabling e10s with some whitelisted addons during 49 and sounds like some of them would be affected by this.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8785230 [details] [diff] [review]
Shimwaiver for l10n. v1

Fix for a sync issue with e10s enabled addons.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fd9e2518fc2

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1d47b6f9e8dc