Dão Gottwald [:dao] Reporter
	Description • 7 years ago

See bug 1054740 comment 36 ff.

	Dão Gottwald [:dao] Reporter
	Comment 1 • 7 years ago

Turns out disabling this test makes test_prefs.py fail.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 7 years ago

Dao, when we replace the tabs, do we wait for the content to be loaded before firing "sessionstore-windows-restored"? I would assume that we don't do that, which could mean the following to Marionette:

We receive "sessionstore-windows-restored" and let our framescript install into the content browser of the first tab. While we do this, the tab is still loading and it might occur a remoteness change. With that the known framescript id is lost, because it got replaced with another one. So Marionette fails to send a command to the framescript.

	Dão Gottwald [:dao] Reporter
	Comment 4 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #3)
> Dao, when we replace the tabs, do we wait for the content to be loaded
> before firing "sessionstore-windows-restored"?

No, we don't.

> I would assume that we don't
> do that, which could mean the following to Marionette:
> 
> We receive "sessionstore-windows-restored" and let our framescript install
> into the content browser of the first tab. While we do this, the tab is
> still loading and it might occur a remoteness change.

Hmm, yes, this might happen if you restore chrome pages.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 5 • 7 years ago

I'm still trying to nail down this problem. While doing so I also noticed bug 1363368, which doesn't always cause this problem but might be a side-effect of a wrong behavior after an in_app restart. I would like to have a look at this problem first.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 7 years ago

Given all the failures on Linux and that bug 1363368 will take me longer to fix (I will be out parts of next week) I would propose we land a follow-up patch to also skip all those tests for Linux.

I will upload a patch tomorrow morning.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 7 • 7 years ago

Actually I will not mark those tests skipped for the Linux and Mac platforms, but get rid of the "browser.startup.page" preference usage. With that the problem should go away given that we do not use sessionrestore for the restarts.

The underlying issue will then completely be covered by bug 1363368.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 10 • 7 years ago

It turns out that tests are still failing even without using the pref `browser.startup.homepage`. Dao, are the code changes you made always executed during startup, whether if sessionrestore is enabled or not? If yes, any way to escape from it? If not we will have to keep the tests disabled for now.

	Dão Gottwald [:dao] Reporter
	Comment 11 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #10)
> Dao, are the code changes you made always
> executed during startup, whether if sessionrestore is enabled or not?

Not that I know.

	David Burns :automatedtester
	Comment 12 • 7 years ago

Dao, Could you help here?  Unfortunately, we don't understand your code here and it has the potential to break a lot of mochitests/reftests (marionette bootstraps mochitests/reftests) and restart tests here. 

If we can't figure it out we may need to backout your patch until we can solve the race condition.

	Dão Gottwald [:dao] Reporter
	Comment 13 • 7 years ago

Sorry, but these tests or the framework are clearly fragile / broken, and if you as the domain expert can't figure out what's going on, I don't see how I could help. As things stand, backing out the patch because of this is not an option.

	David Burns :automatedtester
	Comment 14 • 7 years ago

I really dont understand, your patch created new race conditions as seen in the discussions. The framework is not `clearly fragile / broken`. That attitude is not helpful in the slightest. I could easily claim the same against your code because its created this situation.

Your code has created a situation, either by revealing an underlying bug or created a new issue. Could you help us understand _YOUR_ code so we can fix it instead of attacking us. If this creates issues with mochitests/reftests until it is fixed backout will become an option even if it is a messy option.

	Dão Gottwald [:dao] Reporter
	Comment 15 • 7 years ago

As far as I can see, there's no race condition in the code I touched. It's working as expected. It's the marionette framework that needs a serious overhaul -- if I understand you correctly in bug 1363368 comment 9, you already realized this, so I'm not sure what we're arguing about here.

	Dão Gottwald [:dao] Reporter
	Comment 16 • 7 years ago

Oops, thought I was responding to Henrik when I was referring to bug 1363368 comment 9 etc. Apologies for the confusion. But what I said stands: I think the bug is in marionette, not Firefox.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 17 • 7 years ago

Given that there is no easy solution right now, I will continue in skipping all those tests for OS X and Linux.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 19 • 7 years ago

With the last update of the patch I marked the tests as skipped across all platforms. Reason is that they might even fail on Windows, even they don't yet.

	Andreas Tolfsen ❲:ato❳
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8867089 [details]
Bug 1361837 - Update restart unit tests to not activate sessionrestore.

https://reviewboard.mozilla.org/r/138696/#review143092

	Pulsebot
	Comment 21 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c914393e8ef
Update restart unit tests to not activate sessionrestore. r=ato

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 22 • 7 years ago

So the real underlying fix will not happen on this bug but bug 1363368. So adjusting the summary to reflect the reality.

	Carsten Book [:Tomcat]
	Comment 23 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9c914393e8ef

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 24 • 7 years ago

Dao, so your original changes which replaced the initial tab has been revert now via bug 1365541. So I assume that we can re-enable all the previously skipped restart tests? Well, in case all passes. I'm asking to make sure we do not get another change again. Thanks.

	Dão Gottwald [:dao] Reporter
	Comment 25 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #24)
> Dao, so your original changes which replaced the initial tab has been revert
> now via bug 1365541. So I assume that we can re-enable all the previously
> skipped restart tests? Well, in case all passes. I'm asking to make sure we
> do not get another change again. Thanks.

Further changes are likely given the desire to improve sesssion restore performance, and it would be wise to make these tests or the framework more robust in this regard.