874817 - [Session Restore] Put addon-compat changes behind a kill switch

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

We should put the changes of Bug 867097, Bug 867142 and Bug 873835 behind a kill switch (preference or preprocessor directive) to ensure that we can cancel/postpone them during beta in case of emergency.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

We could re-introduce the old code and put that behind kill switches. We could also just re-introduce those ugly properties we removed and make them behave the same as before - behind a kill switch of course.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Yeah, that's the idea. Do you want to handle this? I suppose I could do it, but that might be somewhat accident-prone.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Kill switch (WIP)

Here's the first draft of a killswitch. The idea is to put all our changes in NonBlockingSessionStore.jsm, keeping OldSessionStore.jsm untouched.

I'm doing something wrong with the configure.in, though, because the wrong file is used atm. Investigating, but if anybody has an idea, I'm interested.

Comment 4

[(no longer active)]

Comment on attachment 753656 [details] [diff] [review]
Kill switch (WIP)

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

I think splitting the files this way is a mistake.  There's going to be fixes that somebody makes to one copy and forgets to port to the other copy, etc.  Why not just use AC_DEFINE like I suggested below and then hide all of the new code behind #ifdef MOZ_NON_BLOCKING_SESSION_RESTORE?

::: configure.in
@@ +1585,5 @@
>  dnl ========================================================
>  MOZ_OS2_HIGH_MEMORY=1
>  MOZ_ARG_DISABLE_BOOL(os2-high-mem,
>  [  --disable-os2-high-mem  Disable high-memory support on OS/2],
> +    ,

Not sure why this is needed.

@@ +8956,5 @@
>  dnl ========================================================
>  
> +MOZ_ARG_DISABLE_BOOL(nonblocking-session-restore,
> +[ --disable-nonblocking-session-restore Disable non-blocking session store.],
> +  ,

Shouldn't you have MOZ_NON_BLOCKING_SESSION_RESTORE= here?

@@ +8960,5 @@
> +  ,
> +  MOZ_NON_BLOCKING_SESSION_RESTORE=1
> +)
> +
> +AC_SUBST(MOZ_NON_BLOCKING_SESSION_RESTORE)

Also, I believe you want something like:

if test -n "$MOZ_NON_BLOCKING_SESSION_RESTORE" ; then
  AC_DEFINE(MOZ_NON_BLOCKING_SESSION_RESTORE)
fi

Comment 5

[(no longer active)]

Also, you don't need a --disable/--enable arg.  You should just be able to use AC_SUBST and AC_DEFINE and then in the mozconfig, export MOZ_NON_BLOCKING_SESSION_RESTORE=1.  If you want to build Firefox with that by default, you can add it to browser/confvars.sh.

Comment 6

[Alex Keybl [:akeybl]]

We probably want to track this for FF24 as well a bug tracking the actual decision.

Got here from https://dutherenverseauborddelatable.wordpress.com/2013/05/23/add-on-breakage-continued-list-of-add-ons-that-will-probably-be-affected/

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> Comment on attachment 753656 [details] [diff] [review]
> Kill switch (WIP)
> 
> Review of attachment 753656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think splitting the files this way is a mistake.  There's going to be
> fixes that somebody makes to one copy and forgets to port to the other copy,
> etc.  Why not just use AC_DEFINE like I suggested below and then hide all of
> the new code behind #ifdef MOZ_NON_BLOCKING_SESSION_RESTORE?

Well, my problem is that these #ifdefs would very quickly start creeping all over the place.
My intention was to use these #ifdefs for each of the blockers of bug 874381, plus for bug 838577. Just for the largest patch of bug 838577, that's ~31 #ifdefs in a single file, most of which cover the whole implementation of a function.

We can do that. I just fear for readability.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Configure support

In that case, here's the logistics, so that related bugs can use
MOZ_NON_BLOCKING_SESSION_RESTORE as a killswitch.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

MOZ_NON_BLOCKING_SESSION_RESTORE is not a good name to put the addon-compat changes behind as they're just about cleanup and will not change anything about the blocking behavior.

I also wonder if having a build-time switch for bug 838577 is really the way to go. We'd have lots of #ifdefs in this one file as David already mentioned and we would need to run tests with MOZ_NON_BLOCKING_SESSION_RESTORE=0 manually.

What about having a specific branch (like for per-window PB) that keeps m-c just without the patches from bug 838577? So we get almost instant test coverage, also for other test suites that might interfere with sessionstore somehow.

Comment 10

[(no longer active)]

Comment on attachment 754784 [details] [diff] [review]
Configure support

(In reply to Tim Taubert [:ttaubert] from comment #9)
> MOZ_NON_BLOCKING_SESSION_RESTORE is not a good name to put the addon-compat
> changes behind as they're just about cleanup and will not change anything
> about the blocking behavior.
> 
> I also wonder if having a build-time switch for bug 838577 is really the way
> to go. We'd have lots of #ifdefs in this one file as David already mentioned
> and we would need to run tests with MOZ_NON_BLOCKING_SESSION_RESTORE=0
> manually.
> 
> What about having a specific branch (like for per-window PB) that keeps m-c
> just without the patches from bug 838577? So we get almost instant test
> coverage, also for other test suites that might interfere with sessionstore
> somehow.

The idea that I had was to hide all of the new stuff behind #ifdef's, adding MOZ_WHATEVER_NAME_WE_END_UP_USING=1 in browser/confvars.sh on mozilla-central, and then maintain a separate branch which only removes that confvars.sh line and runs tests with the alternate config.  Such a branch is very easy to maintain off of a cron script because the confvars.sh changes are the only difference with mozilla-central, so the chances of a merge failing are very low.  This model worked very well for per-window PB.

It's true that code using a lot of #ifdefs is not pleasant to read, but code which duplicates things is _even_ harder to read, IMO, and we've had terrible similar models in our code base which have diverged needlessly (see the in-content prefs code, as an example.)

Anyway, I think you should make up your mind on what approach you want to use before I can provide any meaningful feedback on this patch.  But this small patch looks fine, although it's best to ask review form a build system peer on this.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> The idea that I had was to hide all of the new stuff behind #ifdef's, adding
> MOZ_WHATEVER_NAME_WE_END_UP_USING=1 in browser/confvars.sh on
> mozilla-central, and then maintain a separate branch which only removes that
> confvars.sh line and runs tests with the alternate config.  Such a branch is
> very easy to maintain off of a cron script because the confvars.sh changes
> are the only difference with mozilla-central, so the chances of a merge
> failing are very low.  This model worked very well for per-window PB.

That sounds good to me.

I wonder, why does this bug try to span multiple changes we're planning? To me there is bug 874381 and dependents which might break too many add-ons - which is why we want to have a kill-switch.

On the other side there is bug 838577. This doesn't include any API changes and is supposed to not break add-ons. We want a kill-switch here only because this whole new code might have too many bugs to fix until release.

So shouldn't we have at least two build-time switches then? If the new saveState() stuff turns out to be too buggy I'd rather not disable all the sessionstore cleanup. And vice versa, if too many add-ons break and can't be fixed in time I'd rather not disable the perf gains from bug 838577.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Oh and also I'm pretty sure we don't really need an extra branch for bug 874381 and friends.

Comment 13

[(no longer active)]

I am assuming that comment 11 is a question for David.  :-)

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 754784 [details] [diff] [review]
Configure support

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

::: browser/confvars.sh
@@ +56,5 @@
>  MOZ_MEDIA_NAVIGATOR=1
>  if test "$OS_TARGET" = "WINNT" -o "$OS_TARGET" = "Darwin"; then
>    MOZ_FOLD_LIBS=1
>  fi
> +MOZ_NON_BLOCKING_SESSION_RESTORE=1

I guess you don't want to put this part before FF25. Note you'll have to adjust xulrunner/confvars.sh too.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

I think we decided to WONTFIX this, right?

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Yes, we decided to back out in case things break.