Closed
Bug 1361837
Opened 8 years ago
Closed 8 years ago
Don't use browser.startup.page for restart unit tests, and temporarily skip all in_app restart tests.
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox54 unaffected, firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox54 | --- | unaffected |
firefox55 | --- | fixed |
People
(Reporter: dao, Assigned: whimboo)
References
Details
Attachments
(1 file)
See bug 1054740 comment 36 ff.
Reporter | ||
Comment 1•8 years ago
|
||
Turns out disabling this test makes test_prefs.py fail.
Assignee | ||
Comment 3•8 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.
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 4•8 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.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 5•8 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.
Depends on: 1363368
Assignee | ||
Comment 6•8 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.
Assignee | ||
Comment 7•8 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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•8 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.
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 11•8 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.
Flags: needinfo?(dao+bmo)
Comment 12•8 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.
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 13•8 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.
Flags: needinfo?(dao+bmo)
Comment 14•8 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.
Updated•8 years ago
|
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 15•8 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.
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 16•8 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.
Assignee | ||
Comment 17•8 years ago
|
||
Given that there is no easy solution right now, I will continue in skipping all those tests for OS X and Linux.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•8 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.
Assignee | ||
Updated•8 years ago
|
Attachment #8867089 -
Flags: review?(ato)
Comment 20•8 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
Attachment #8867089 -
Flags: review?(ato) → review+
Comment 21•8 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
Assignee | ||
Comment 22•8 years ago
|
||
So the real underlying fix will not happen on this bug but bug 1363368. So adjusting the summary to reflect the reality.
status-firefox54:
--- → unaffected
status-firefox55:
--- → affected
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.
Comment 23•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 24•8 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.
Flags: needinfo?(dao+bmo)
Reporter | ||
Comment 25•8 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.
Flags: needinfo?(dao+bmo)
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•