Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restartlessness and e10s #489

Closed
3 tasks done
myrdd opened this issue Oct 6, 2014 · 21 comments
Closed
3 tasks done

restartlessness and e10s #489

myrdd opened this issue Oct 6, 2014 · 21 comments

Comments

@myrdd
Copy link
Member

myrdd commented Oct 6, 2014

The aim is to to

  1. convert RP into a restartless extension (aka bootstrapped)
  2. support multiprocess firefox (aka Electrolysis or e10s)

Development branch: dev-restartless-and-e10s

As of 2014-12-02 the main work is already done, so the aim is to find and remove all remaining bugs.

  • implement restartlessness
  • implement e10s support
  • testing and finding bugs – known issues

IMHO it's great if you don't need to restart firefox when you install/disable/enable the addon. But the main reason I want to do is that overlay extensions are not supported by the Add-on Debugger, which is built-in since ff 31.

I think it should be possible. A tutorial how to convert can be found here. As RP also implements an XPCOM component (nsIContentPolicy), we need to consider this.

@ldgbc
Copy link

ldgbc commented Oct 15, 2014

This sound like a big project that would require to rewrite most of the code, if it going to be "rewritten" it should be design so that Electrolysis (e10s) can be implement in the future.

Perhaps making it restartless and e10s at the same thing would be the better path.

There is a Bugzilla ID here for e10s; https://bugzilla.mozilla.org/show_bug.cgi?id=1077185
The message was for "Justin", guess Chris isn't aware of the change in ownership yet.

@myrdd
Copy link
Member Author

myrdd commented Oct 15, 2014

Thanks a lot for the info. In fact I'm almost finished converting into restartless. Supporting e10s is important of course! I'll have a look at how to support it.

Note that I've so far only worked on RP 1.0 beta which had been started (and discontinued) by Justin. On AMO it's v0.5. I hope RP 1.0 will become stable before e10s is enabled by default in ff.

@myrdd
Copy link
Member Author

myrdd commented Oct 26, 2014

I've converted RP to restartless, see 40fca25 (it's a very big commit; many file renames and code styling changes couldn't be detected). The commit message is:

converting RequestPolicy to restartless

see https://developer.mozilla.org/en-US/Add-ons/How_to_convert_an_overlay_extension_to_restartless

in addition, the following has changed:
- javascript filenames renamed to lowercase.
- all javascript files moved to src/content/
    (also default preferences)
- removed unused "packaging" chrome manifest
- move xul files and their javascript files to "ui" subdir
- the structure of overlay.xul is now in src/lib/xul-trees.js .
  The module src/lib/xul-utils.jsm provides functions to add the XUL elements to
  a window or remove them.
- The locale strings used in overlay.xul have been moved from the *.dtd files
  into the requestpolicy.properties files.
- use a Makefile instead of ant for building

I created a separate branch for it so that you can test it. I'll implement mozmill tests and probably also support e10s (see above) before I put it on dev-1.0.

Please test this branch and tell me if it works for you.

@myrdd myrdd added this to the 1.0beta8 milestone Oct 26, 2014
@myrdd myrdd modified the milestones: 1.0.beta9, 1.0.beta8 Nov 6, 2014
@myrdd myrdd changed the title convert RP to be restartless convert RP to be restartless and support multiprocess firefox Nov 19, 2014
@myrdd
Copy link
Member Author

myrdd commented Nov 25, 2014

I've started splitting commit 40fca25. I've pushed a big number of refactoring commits to dev-1.0. I hope that restartlessness and e10s support are finished for christmas.

@myrdd
Copy link
Member Author

myrdd commented Nov 25, 2014

I've created the dev-restartless-rebased branch which rebases dev-restartless on dev-1.0. You can now better see the changes because git detected the file renames. Therefore also blaming should work well. I'll keep dev-restartless for reference purposes.

@ldgbc
Copy link

ldgbc commented Nov 26, 2014

Do we report problem with this branch here or create new issue? Here some issue I notice.

Using RP1.0beta8.1 // Firefox 33.1.1

(1) Conflicting when using Normal Window (NW) and Private Window (PW) (Ctrl+Shirt+P)
Open a Firefox NW and another PW. Choose PW then,
Click RequestPolicy (RP), clicking on "Help, Manage Policies, Preferences, Request Log" will open a new tab in the NW rather than the PW

(2) Restartless only if RP is not already installed.
Create new profile. Install RP 1.0beta8.1 master branch, restart add-on.
Install "dev-restartless-rebased" branch. Add-on manager will ask to be Restart before it install. However it is restartless if RP 1.0beta8.1 isn't installed already. I think Adblock Plus manage to overcome this problem when they convert to restartless/jetpack.

(3) PW's control NW RequestPolicy
Open, https://www.mozilla.org/en-US/firefox/new/ in a NW.
Open a PW, click RP, you will see RP display the Mozilla list even though you're on a "Blank Private Browsing" tab, clicking on "Allow Mozilla.org to Mozilla.net" will refresh https://www.mozilla.org/en-US/firefox/new/ in the NW.

This problem can (sometime) be solve by enabling the following: opening the Preferences > Basic > Menu
Allow adding non-temporary rules when using Private Browsing Mode
Does not always work.

@myrdd
Copy link
Member Author

myrdd commented Nov 26, 2014

Thanks for your report @ldgbc, I think it's ok to post issues here.

Thank you for testing. I'm also finding bugs from time to time – note that I'll rebase occasionally rebase dev-restartless-rebased again and again, but I'll write a comment in that case.

@myrdd myrdd changed the title convert RP to be restartless and support multiprocess firefox restartlessness and e10s Dec 2, 2014
@myrdd
Copy link
Member Author

myrdd commented Dec 2, 2014

I've made some commits, rebased again and pushed everything including the current dev state of e10s as well. Everything is now in the dev-restartless-and-e10s branch, I've deleted the other branches. From now on I won't rebase the code again as it's already too far away from dev-1.0.

Btw, I've summarized the current dev state in the first post (for anyone visiting this issue for the first time).

@myrdd
Copy link
Member Author

myrdd commented Dec 2, 2014

@ldgbc I changed my mind and will create separate issues for your bugreports. If you find more bugs, please also create a new issue for them, I will attach labels appropiately.

For every new issue, please start the issue's topic with [restartless/e10s] and add as the first line of the issue the following Markdown code:

Issue considering the development branch [`dev-restartless-and-e10s`](https://github.com/RequestPolicyContinued/requestpolicy/tree/dev-restartless-and-e10s), see #489.

Thank you!

@myrdd
Copy link
Member Author

myrdd commented Dec 2, 2014

(2) Restartless only if RP is not already installed.

@ldgbc this is not a bug. Read:

Also note that once you do get this all up and running, your users will still have to restart Firefox once to install your first restartless update. While your new add-on may not need a restart to install, if you're updating from an old version that is not restartless then it will need a restart to uninstall that first.

Comment is from here.

@myrdd
Copy link
Member Author

myrdd commented Jan 4, 2015

There are still several issues. I'm not documenting them online, but I'm working on fixing them.

@myrdd
Copy link
Member Author

myrdd commented Jan 8, 2015

To show you I'm still there and working on the problems, I've pushed a bunch of commits to dev-restartless-and-e10s, see here.

The main remaining issue is that RP does not cleanly shut down yet, i.e. when „disable“ is clicked on about:addons.

@myrdd
Copy link
Member Author

myrdd commented Jan 12, 2015

Sadly the restartless/e10s brach is still too buggy to be merged into dev-1.0. I'm still working on the startup/shutdown process working cleanly. After that I'll fix the bugs that have been reported – thank you @nysatrok and @ldgbc for testing and reporting the issues! 👍

@myrdd
Copy link
Member Author

myrdd commented Jan 15, 2015

I'm still working on the startup/shutdown process working cleanly.

I think this is done, at least I don't get any errors when I enable/disable the addon.

  • The commit I've just pushed is 75aca76.
  • I've added some documentation about the „Environment System“. I think it's too text heavy, so some day I might add some diagrams.

Now it's time to fix the more obvious bugs that have been reported.

@myrdd
Copy link
Member Author

myrdd commented Jan 24, 2015

My current plan is to merge the dev-restartless-and-e10s branch as soon as it works well when E10s is disabled. Real E10s support is then another task, but currently there are more important issues, which is the reason why I want to merge that branch back to dev-1.0.

@myrdd
Copy link
Member Author

myrdd commented Feb 15, 2015

I've just pushed some more commits.

Sorry for being silent recently, it's examination period. As finishing this issue is still the next step before releasing RPC to AMO, it would be great if you @ldgbc @nodiscc could do some testing if you can bring up the time. Other testers are also welcome; creating the XPI should be fairly easy.

@nodiscc
Copy link
Contributor

nodiscc commented Feb 16, 2015

@myrdd could you release a dev-restartless-and-e10s XPI at https://github.com/RequestPolicyContinued/requestpolicy/releases (and explicitely mark it as "development version for testers)? I don't have access to my dev/build machine right now.

@nodiscc
Copy link
Contributor

nodiscc commented Mar 20, 2015

@myrdd time for a new release? :)

@myrdd
Copy link
Member Author

myrdd commented Mar 21, 2015

@nodiscc working on it. I want to make a better workaround for #447 for that release.

@ldgbc
Copy link

ldgbc commented Mar 22, 2015

Not sure if there much testing to be done. I don't see any problem with it except for one that I'll report after this post.

Question though, is the https://github.com/RequestPolicyContinued/requestpolicy/tree/dev-restartless-and-e10s consider dead now? I notice the merge to dev1.0 built and it more updated by commit counts.

@nodiscc you could just make an XPI using any software that can create a ZIP files, that how I build RP at least.

@myrdd
Copy link
Member Author

myrdd commented Mar 22, 2015

You are right @ldgbc, I've now removed the dev-restartless-and-e10s branch.

@nodiscc you could just make an XPI using any software that can create a ZIP files, that how I build RP at least.

That could work, but might not work anymore in future. I've introduced a preprocessor, which is used for example in the Logger, see here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants