[Advance warning] Session Restore is changing, will break add-ons

[David Rajchenbach-Teller]

As part of project Async, we have been working on refactoring Firefox’
Session Restore to ensure that it does not block the main thread. Part
of the work has been cleaning up the code and the data structures
involved in Session Restore both to give us some maneuverability and to
improve the chances of catching refactoring errors.

Unfortunately, a large number of add-ons seem to rely upon these
undocumented data structures. Some of their features might therefore
stop working. If you are the author of one such add-on, you should
monitor carefully bug 874381 and its blockers. If you realize that we
are about to break your add-on, please inform us asap, so that we can
work out a solution.

Cheers,
David
--
David Rajchenbach-Teller, PhD
Performance Team, Mozilla

[Kyle Huey]

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>

This is probably not the ideal mailing list to send this to.

- Kyle

[Lawrence Mandel]

cc Jorge Villalobos.

[Jorge Villalobos]

Thank you for the poke. I was informed about it this morning and we'll
be keeping track of it and informing add-on devs in due time.

Jorge

[Jorge Villalobos]

On 5/21/13 10:01 AM, Lawrence Mandel wrote:

[Ehsan Akhgari]

On 2013-05-21 9:07 AM, David Rajchenbach-Teller wrote:
> As part of project Async, we have been working on refactoring Firefox’
> Session Restore to ensure that it does not block the main thread. Part
> of the work has been cleaning up the code and the data structures
> involved in Session Restore both to give us some maneuverability and to
> improve the chances of catching refactoring errors.
>
> Unfortunately, a large number of add-ons seem to rely upon these
> undocumented data structures. Some of their features might therefore
> stop working. If you are the author of one such add-on, you should
> monitor carefully bug 874381 and its blockers. If you realize that we
> are about to break your add-on, please inform us asap, so that we can
> work out a solution.

Do we have a kill switch for the new stuff (a build-time flag or a
runtime pref) which we can use to turn this off on Beta if the add-on
compatibility problem proves to be bad enough that we would need to wait
for a while before we can ship this? I have experience maintaining a
branch to build and test Firefox with per-window private browsing turned
off at build-time which I used when Firefox 20 migrated through our
release channel and finally shipped. I would be happy to help you on
doing the same thing if needed.

Cheers,
Ehsan

[David Rajchenbach-Teller]

Unfortunately, we do not.

For the current batch of clean-up changes, it is certainly possible to
add a kill switch. Time-consuming, certainly not nice (the kill switch
will creep in in dozens of places in the code, if not hundreds), but
possible.

For the upcoming set of rewrite-half-of-the-code changes, though, having
a kill switch pretty much means forking the code of Session Restore into
an "old" session restore and a new one.

Do we have a policy on these things?

Cheers,
David

On 5/22/13 5:16 AM, Ehsan Akhgari wrote:
> Do we have a kill switch for the new stuff (a build-time flag or a
> runtime pref) which we can use to turn this off on Beta if the add-on
> compatibility problem proves to be bad enough that we would need to wait
> for a while before we can ship this? I have experience maintaining a
> branch to build and test Firefox with per-window private browsing turned
> off at build-time which I used when Firefox 20 migrated through our
> release channel and finally shipped. I would be happy to help you on
> doing the same thing if needed.
>
> Cheers,
> Ehsan
>

[Johnathan Nightingale]

On May 22, 2013, at 5:33 AM, David Rajchenbach-Teller wrote:

> Unfortunately, we do not.
>
> For the current batch of clean-up changes, it is certainly possible to
> add a kill switch. Time-consuming, certainly not nice (the kill switch
> will creep in in dozens of places in the code, if not hundreds), but
> possible.
>
> For the upcoming set of rewrite-half-of-the-code changes, though, having
> a kill switch pretty much means forking the code of Session Restore into
> an "old" session restore and a new one.
>
> Do we have a policy on these things?

Policy[1] is that whenever something lands on central, it is the developer's responsibility to provide for the ability to turn it off. Usually that's just a kill switch in cases where it makes sense, or a backout where the patch is unlikely to accumulate much change on top of itself in a given release.

In cases where neither of those works (Ehsan's private browsing changes, or dmandelin's landing of ionmonkey in FF18) engineers have had to work harder to maintain that ability, e.g. by maintaining a shadow tree that doesn't have their patches, but has all the other landings. That's what Ehsan's pointing to in his reply.

I agree with Ehsan that, one way or another, we'll need to be able to disable these changes if they cause too much bustage (though I'm very happy to know that we're cleaning up that code in general).

J


[1] http://mozilla.github.io/process-releases/draft/development_overview/ Ancient, and shows it, but still relevant for this case. See "Moving work from one channel to another"

---
Johnathan Nightingale
VP Firefox Engineering
@johnath

[David Rajchenbach-Teller]

Opening Bug 874817 to add that kill switch.

Just for clarification: we might kill add-ons that specifically look at
the contents of undocumented private data structures. The advance
warning is here because we know that some such add-ons exist.

Given that all these refactorings take place on a single file, it might
make sense to just backout the changes if necessary.

Cheers,
David

On 5/22/13 3:35 PM, Johnathan Nightingale wrote:
> Policy[1] is that whenever something lands on central, it is the
> developer's responsibility to provide for the ability to turn it off.
> Usually that's just a kill switch in cases where it makes sense, or a
> backout where the patch is unlikely to accumulate much change on top of
> itself in a given release.
>
> In cases where neither of those works (Ehsan's private browsing changes,
> or dmandelin's landing of ionmonkey in FF18) engineers have had to work
> harder to maintain that ability, e.g. by maintaining a shadow tree that
> doesn't have their patches, but has all the other landings. That's what
> Ehsan's pointing to in his reply.
>
> I agree with Ehsan that, one way or another, we'll need to be able to
> disable these changes if they cause too much bustage (though I'm very
> happy to know that we're cleaning up that code in general).
>
> J
>
>
> [1] http://mozilla.github.io/process-releases/draft/development_overview/ Ancient,
> and shows it, but still relevant for this case. See "Moving work from
> one channel to another"
>
> ---
> Johnathan Nightingale
> VP Firefox Engineering
> @johnath
>

[Tim Taubert]

I talked to Gavin yesterday and we think the best approach would be to
back out the Session Restore changes for now as they don't provide a
real benefit other than code cleanup (and don't block any other work).

The plan would then be to re-land them *with* a kill switch in the same
cycle that brings Australis - so we would need to prepare and keep those
patches ready. The reasoning is that we indeed will break different
add-ons than Australis but at least there will only be one release with
a couple of add-ons breaking instead of two in a row.

For bug 838577 we will probably need to maintain a shadow tree as
Johnathan mentioned. I would suggest we talk to Ehsan as he has
experience in doing this successfully.

- Tim

Tim Taubert
Firefox Engineer
ttau...@mozilla.com

[Ehsan Akhgari]

On 2013-05-23 2:45 AM, Tim Taubert wrote:
> I talked to Gavin yesterday and we think the best approach would be to
> back out the Session Restore changes for now as they don't provide a
> real benefit other than code cleanup (and don't block any other work).
>
> The plan would then be to re-land them *with* a kill switch in the same
> cycle that brings Australis - so we would need to prepare and keep those
> patches ready. The reasoning is that we indeed will break different
> add-ons than Australis but at least there will only be one release with
> a couple of add-ons breaking instead of two in a row.
>
> For bug 838577 we will probably need to maintain a shadow tree as
> Johnathan mentioned. I would suggest we talk to Ehsan as he has
> experience in doing this successfully.

Thanks for the due diligence here! I would be more than happy to help.
Please ping me when you're ready!

Cheers,
Ehsan

[David Rajchenbach-Teller]

On 5/23/13 8:45 AM, Tim Taubert wrote:
> I talked to Gavin yesterday and we think the best approach would be to
> back out the Session Restore changes for now as they don't provide a
> real benefit other than code cleanup (and don't block any other work).
>
> The plan would then be to re-land them *with* a kill switch in the same
> cycle that brings Australis - so we would need to prepare and keep those
> patches ready. The reasoning is that we indeed will break different
> add-ons than Australis but at least there will only be one release with
> a couple of add-ons breaking instead of two in a row.

Yes, I believe that's best.

> For bug 838577 we will probably need to maintain a shadow tree as
> Johnathan mentioned. I would suggest we talk to Ehsan as he has
> experience in doing this successfully.

I'll get started on that. Expect the complicated patch to become more
complicated :)

Cheers,
David

> - Tim