1129003 - [e10s] Stop using shims in page-mod

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Dave Townsend [:mossop]]

page-mod works in e10s mode but is still using some shims. This will be slowing down page loads and can cause other odd problems like bug 1124703

Comment 1

[Dave Townsend [:mossop]]

I think we probably need to up this to be more urgent given bug 1129567. CPOW logs show that when Amazon pages load with even a simple page-mod in place we create a metric ton of CPOW traffic.

Comment 2

[Dave Townsend [:mossop]]

Attachment: pull request

Want to do a review here? Tests are failing on travis due to bug 1132964 but that's a test only issue I can fix in the test itself if I can't fix it in Firefox, no reason not to start the review now.

With this in place page-mod only uses a small amount of shims which is difficult to work around due to bug 1132767 but I might be able to fix that in a follow-up too.

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8564279 [details] [review]
pull request

Any chance you've got the time to do the review here erik? This is pretty important for e10s support, particularly as it adds APIs allowing us and add-on developers to interact with the child processes correctly.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8564279 [details] [review]
pull request

Please see the pull request, let me know if you have any questions.

The main thing in my mind is getting tests for the destruction of the ChildPageMod and WorkerChild instances.  Three cases come to mind to test:

* add-on unload
* page mod is manually destroyed
* associated window unloads

I don't see any unloading tests for sdk/content/context-menu either.

I can r+ large parts of this, maybe we can break this up in to smaller pieces?

Comment 5

[Dave Townsend [:mossop]]

Attachment: updated pull request

This is a new pull request, rebased to master and split out into commits that hopefully will make reviewing easier. The pull request has notes on the breakdown and what has been addressed from the previous review. Please let me know if you have any questions.

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 8570294 [details] [review]
updated pull request

Looks good to me, we can add more tests later.  I'm curious if you expect this implementation of page-mod will work on mobile though?

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8570294 [details] [review]
updated pull request

Good catch. So interestingly since bug 1058698 page-mod has been entirely broken in Fennec. Thankfully bug 1129567 has been reverting that on aurora and down so that hasn't affected any release users. The changes here are almost entirely mobile compatible but there was one small bug that I've fixed that gets page-mod working on Fennec again.

I've added three new changesets that I'd like you to look over before I squash and rebase to land, should be pretty quick I hope.

The first renames a couple of properties in the remote APIs. It was a little confusing having a browser and isBrowser property which have different meanings so I renamed to frameElement and isTab.

The second fixes the Fennec issue by using the utils module to try to get a tab for a browser element instead of relying on Firefox specific code.

The third implements the processID test you asked for.

One of the tests failed during one run on travis but passed in the rest, if that's a problem I can disable that test and investigate after this has landed.

Comment 8

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a7317a880b0d8a411b6069f0ec1f323aa0274418
Bug 1129003: Make page-mod page detection run in content processes and use the remote APIs. r=erikvold