Closed Bug 1361837 Opened 3 years ago Closed 3 years ago

Don't use browser.startup.page for restart unit tests, and temporarily skip all in_app restart tests.

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox54 unaffected, firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: dao, Assigned: whimboo)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

Turns out disabling this test makes test_prefs.py fail.
Duplicate of this bug: 1361817
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.
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
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.
Depends on: 1363368
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.
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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
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.
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
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.
Flags: needinfo?(dao+bmo)
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.
Given that there is no easy solution right now, I will continue in skipping all those tests for OS X and Linux.
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.
Comment on attachment 8867089 [details]
Bug 1361837 - Update restart unit tests to not activate sessionrestore.

https://reviewboard.mozilla.org/r/138696/#review143092
Attachment #8867089 - Flags: review?(ato) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c914393e8ef
Update restart unit tests to not activate sessionrestore. r=ato
So the real underlying fix will not happen on this bug but bug 1363368. So adjusting the summary to reflect the reality.
Summary: Fix and re-enable test_quit_restart.py to deal with bug 1054740's changes → Don't use browser.startup.page for restart unit tests, and temporarily skip all in_app restart tests.
https://hg.mozilla.org/mozilla-central/rev/9c914393e8ef
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
You need to log in before you can comment on or make changes to this bug.