Closed
Bug 1338911
Opened 7 years ago
Closed 7 years ago
Permaorange in test_refresh_firefox.py TestFirefoxRefresh.testReset when Gecko 53 merges to beta 2017-03-06 due to bug 1316330
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox-esr45 unaffected, firefox51 unaffected, firefox52 unaffected, firefox-esr52 unaffected, firefox53+ verified, firefox54 fixed)
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox-esr45 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox53 | + | verified |
firefox54 | --- | fixed |
People
(Reporter: philor, Assigned: jessica)
References
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
2.95 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
2.95 KB,
patch
|
Details | Diff | Splinter Review |
aurora-as-beta simulation in https://treeherder.mozilla.org/#/jobs?repo=try&revision=279094aebc8c942cd8b3b94a4e11d8f5f866146b says that https://hg.mozilla.org/releases/mozilla-aurora/rev/13ef12e80f26e0c1cec5e862fe2faa9940306c7d is going to cause Marionette permaorange when we merge to beta, https://treeherder.mozilla.org/logviewer.html#?job_id=76691164&repo=try [Tracking Requested - why for this release]: permaorange, closed tree.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(jjong)
Assignee | ||
Comment 1•7 years ago
|
||
Enabling debug I get this error: JavaScript error: data:application/javascript,(function () { content.document.getElementById("errorTryAgain").click(); })(), line 1: TypeError: content.document.getElementById(...) is null The reason is because, in RELEASE_OR_BETA, when starting the browser a modal dialog shows up to ask the user to set it to the default browser. After bug 1316330, we block events and scripts for executing when entering modal state [1], so this part of the test [2] goes wrong. Actually, 'browser.shell.checkDefaultBrowser' is set to false when running marionette tests [3], but this test does a reset, and it seems not to re-load these prefs anymore. Maybe we should reload the prefs? [1] http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/dom/base/nsDocument.cpp#9365 [2] http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/browser/components/migration/tests/marionette/test_refresh_firefox.py#284 [3] http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/testing/marionette/client/marionette_driver/geckoinstance.py#416
Comment 2•7 years ago
|
||
(In reply to Jessica Jong [:jessica] from comment #1) > Enabling debug I get this error: > > JavaScript error: data:application/javascript,(function () { > content.document.getElementById("errorTryAgain").click(); > })(), line 1: TypeError: content.document.getElementById(...) is null > > The reason is because, in RELEASE_OR_BETA, when starting the browser a modal > dialog shows up to ask the user to set it to the default browser. After bug > 1316330, we block events and scripts for executing when entering modal state > [1], so this part of the test [2] goes wrong. What does "block events and scripts for executing" mean in this context? Clearly the script is still running, or we wouldn't see that error. We can programmatically interact with the dialog, or set the pref once the browser is up (but then we'll probably race with the dialog), or we can change the code at http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/browser/components/migration/tests/marionette/test_refresh_firefox.py#375-383 (and the relevant marionette code) so it can set other prefs besides the marionette.* ones, or we can teach the marionette framework to immediately ensure we don't show this dialog as soon as the marionette code runs. The last option seems like the simplest. > Actually, 'browser.shell.checkDefaultBrowser' is set to false when running > marionette tests [3], but this test does a reset, and it seems not to > re-load these prefs anymore. Maybe we should reload the prefs? Part of the 'point' of the reset/refresh is that prefs are reset to their default values as custom preference sets can cause the problems that make people want a reset in the first place. We shouldn't copy prefs like this during the reset. Teaching the marionette framework to ensure these prefs are set after a restart seems sane, though.
Comment 3•7 years ago
|
||
(In reply to :Gijs from comment #2) > during the reset. Teaching the marionette framework to ensure these prefs > are set after a restart seems sane, though. Therefore enforce_gecko_prefs() exists on the marionette instance: http://marionette-client.readthedocs.io/en/latest/reference.html#marionette_driver.marionette.Marionette.enforce_gecko_prefs Maybe this is what you want here?
Comment 4•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #3) > (In reply to :Gijs from comment #2) > > during the reset. Teaching the marionette framework to ensure these prefs > > are set after a restart seems sane, though. > > Therefore enforce_gecko_prefs() exists on the marionette instance: > http://marionette-client.readthedocs.io/en/latest/reference. > html#marionette_driver.marionette.Marionette.enforce_gecko_prefs > > Maybe this is what you want here? No, because it looks like it doesn't set the prefs but kills the browser, then sets the prefs and then restarts, which would break the test.
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to :Gijs from comment #2) > > What does "block events and scripts for executing" mean in this context? > Clearly the script is still running, or we wouldn't see that error. Hmm, good point. What I see is that about:welcomeback does not show (but the title is "Success!") until the modal dialog is dismissed, that's why content.document.getElementById(...) is null. So, the frame script is indeed executed, need to look into this deeper. > Part of the 'point' of the reset/refresh is that prefs are reset to their > default values as custom preference sets can cause the problems that make > people want a reset in the first place. We shouldn't copy prefs like this > during the reset. Teaching the marionette framework to ensure these prefs > are set after a restart seems sane, though. Do you have any hint on how to teach the marionette framework to ensure these prefs are set after a restart? Should we modify marionette.restart()? (I can't find the definition of it...)
Flags: needinfo?(jjong)
Flags: needinfo?(hskupin)
Flags: needinfo?(gijskruitbosch+bugs)
Comment 6•7 years ago
|
||
(In reply to Jessica Jong [:jessica] from comment #5) > Do you have any hint on how to teach the marionette framework to ensure > these prefs are set after a restart? Should we modify marionette.restart()? > (I can't find the definition of it...) You can only do that with enforce_gecko_prefs() which I have mentioned above and which includes a restart. Just using restart() will overwrite changed preferences with the harness default preferences. (In reply to :Gijs from comment #4) > No, because it looks like it doesn't set the prefs but kills the browser, > then sets the prefs and then restarts, which would break the test. You are free to also set the required preferences before calling enforce_gecko_prefs() via self.marionette.set_pref[s]().
Flags: needinfo?(hskupin)
Comment 7•7 years ago
|
||
I think there's no guarantee that the marionette pref set comes 'in time' to avoid the startup dialog. I also assume that this test is fairly unique in that we don't have any other places where we restart the browser *and* wipe all the preferences. I think the most foolproof way to fix this would be to update the test to, in a bit of chrome-level script: 1) set the pref (in case the dialog hasn't shown yet) 2) run some window manager code that checks for the dialog and cancels it if necessary. ... but at a deeper level it feels like the content script might need to ensure it waits for about:welcomeback to either be loaded and/or to load, and then send a message to the parent, on which we wait to then run the tab state flushing. We can use an async code run to then make the python code wait for all of that to have finished. Jessica, can you take this?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jjong)
Assignee | ||
Comment 8•7 years ago
|
||
Thanks :Gijs for the suggestions. I've never written a marionette python test, but can give it a try. :)
Assignee: nobody → jjong
Flags: needinfo?(jjong)
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8837473 [details] [diff] [review] patch, v1. Review of attachment 8837473 [details] [diff] [review]: ----------------------------------------------------------------- I'm not setting any prefs after browser reset, because it'd be too late. So, just dismissing the dialog, if any, and wait for load event if page is not loaded completely yet.
Attachment #8837473 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 11•7 years ago
|
||
try run on aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9212fcb845b894c13f3c3406ec19ad9a427e5c19 try run on aurora-as-beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d265999c83a6638186ce9a2e1e39764a3a2af3d7
Comment 12•7 years ago
|
||
Comment on attachment 8837473 [details] [diff] [review] patch, v1. Review of attachment 8837473 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/migration/tests/marionette/test_refresh_firefox.py @@ +283,5 @@ > }); > }, false); > + > + // If page is not loaded, wait for a load event from content. > + if (content.document.readyState === "complete") { You're using a CPOW here in e10s - content.document isn't available in the parent process, in which this code runs. Instead, we should just always load a frame script that checks the readyState, and if it's ready, click the button we want, and if not, wait for the load of the document and then click the button immediately. Re-reading this code, we don't need to wait for another message or anything, because clicking the button will trigger the session restore which will eventually trigger the SSWindowStateReady with a tab flush which then resolves the script.
Attachment #8837473 -
Flags: review?(gijskruitbosch+bugs) → review-
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to :Gijs from comment #12) > Comment on attachment 8837473 [details] [diff] [review] > patch, v1. > > Review of attachment 8837473 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/migration/tests/marionette/test_refresh_firefox.py > @@ +283,5 @@ > > }); > > }, false); > > + > > + // If page is not loaded, wait for a load event from content. > > + if (content.document.readyState === "complete") { > > You're using a CPOW here in e10s - content.document isn't available in the > parent process, in which this code runs. Oh right, I wonder why it works... > > Instead, we should just always load a frame script that checks the > readyState, and if it's ready, click the button we want, and if not, wait > for the load of the document and then click the button immediately. > Re-reading this code, we don't need to wait for another message or anything, > because clicking the button will trigger the session restore which will > eventually trigger the SSWindowStateReady with a tab flush which then > resolves the script. Yes, I think that'll do. Will upload a new patch for this. Thanks.
Assignee | ||
Comment 14•7 years ago
|
||
Addressed comment 12.
Attachment #8837473 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8837947 -
Flags: review?(gijskruitbosch+bugs)
Comment 15•7 years ago
|
||
(In reply to Jessica Jong [:jessica] from comment #13) > > You're using a CPOW here in e10s - content.document isn't available in the > > parent process, in which this code runs. > > Oh right, I wonder why it works... Interesting. Talking to Andreas yesterday he told me that we have the unsafe CPOW checks enabled for Marionette which should crash Firefox in such cases. Now I wonder if we really have that enabled for Marionette tests. Andreas, can you please check?
Flags: needinfo?(ato)
Comment 16•7 years ago
|
||
Comment on attachment 8837947 [details] [diff] [review] patch for mozilla-aurora. Review of attachment 8837947 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks!
Attachment #8837947 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 17•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #15) > (In reply to Jessica Jong [:jessica] from comment #13) > > > You're using a CPOW here in e10s - content.document isn't available in the > > > parent process, in which this code runs. > > > > Oh right, I wonder why it works... > > Interesting. Talking to Andreas yesterday he told me that we have the unsafe > CPOW checks enabled for Marionette which should crash Firefox in such cases. > Now I wonder if we really have that enabled for Marionette tests. Andreas, > can you please check? Well, one explanation could be that we are loading about:welcomeback in the parent process, in which case content.document won't be a CPOW. The attached patch is still better because initially the tab will be remote (so there'd be a race condition in which it *would* be a CPOW and/or crash) and because we will probably eventually want to move about:welcomeback to load in the child. Anyway, that might be why it's not crashing reliably here. I'll leave Andreas to confirm - though it should be relatively easy to check locally, by echo'ing/printing/whatever the value of gBrowser.selectedBrowser.isRemoteBrowser just before accessing content.document.
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to :Gijs from comment #17) > > Well, one explanation could be that we are loading about:welcomeback in the > parent process, in which case content.document won't be a CPOW. The attached > patch is still better because initially the tab will be remote (so there'd > be a race condition in which it *would* be a CPOW and/or crash) and because > we will probably eventually want to move about:welcomeback to load in the > child. Anyway, that might be why it's not crashing reliably here. I'll leave > Andreas to confirm - though it should be relatively easy to check locally, > by echo'ing/printing/whatever the value of > gBrowser.selectedBrowser.isRemoteBrowser just before accessing > content.document. Yes, about:welcomeback is loaded in the parent process, isRemoteBrowser is returning false. About "blocking events and scripts when entering modal state", this is about scriptloader only, to ensure <script> element's are aren't too early, that means load event for the page is also postponed.
Assignee | ||
Comment 20•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d8cf6df2542df4626585aea9573605da2ce7799&group_state=expanded
Keywords: checkin-needed
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #21) > Needs rebasing. oh sorry, that patch was for mozilla-aurora, will provide a patch for mozilla-central.
Assignee | ||
Comment 23•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8837947 -
Attachment description: patch, v2. → patch for mozilla-aurora.
Assignee | ||
Comment 24•7 years ago
|
||
Comment on attachment 8837947 [details] [diff] [review] patch for mozilla-aurora. Approval Request Comment [Feature/Bug causing the regression]: bug 1316330 [User impact if declined]: Permaorange in test_refresh_firefox.py on beta or release [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes, locally [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: fixes a test case [String changes made/needed]: none
Attachment #8837947 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 25•7 years ago
|
||
Comment on attachment 8837947 [details] [diff] [review] patch for mozilla-aurora. test-only patches don't need approval
Attachment #8837947 -
Flags: approval-mozilla-aurora?
Comment 26•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/87d88eed04b2 Ensure page is loaded before proceeding in test_refresh_firefox.py. r=Gijs
Keywords: checkin-needed
Updated•7 years ago
|
Whiteboard: [checkin-needed-aurora]
Comment 27•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/35ef506e005a3bc7f42637debfada1d3e09d7e5a
Whiteboard: [checkin-needed-aurora]
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/87d88eed04b2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 29•7 years ago
|
||
Hi Phil, could you run aurora-as-beta again to see if this fixes the issue? Thanks.
Flags: needinfo?(philringnalda)
Reporter | ||
Comment 30•7 years ago
|
||
Sure, https://treeherder.mozilla.org/#/jobs?repo=try&revision=0560b48d768f47f54bb6f37cc549e3078dbbb721&group_state=expanded
Flags: needinfo?(philringnalda)
Assignee | ||
Comment 31•7 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #30) > Sure, > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=0560b48d768f47f54bb6f37cc549e3078dbbb721&group_state=e > xpanded Thanks :)
Updated•7 years ago
|
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•