Closed Bug 907062 Opened 11 years ago Closed 10 years ago

Intermittent downloads/test/browser/browser_basic_functionality.js | Test timed out

Categories

(Toolkit :: Downloads API, defect)

Product:
Toolkit
Component:
Downloads API
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME
Milestone:
mozilla26
Tracking Flags:
Tracking Status
firefox24 --- unaffected
firefox25 --- unaffected
firefox26 --- fixed

People

(Reporter: Paolo, Assigned: enndeakin)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(3 files, 2 obsolete files)

Screenshot 10.6
(deleted), image/png
Details
Screenshot 10.7
(deleted), image/png
Details
basic_functionality_fix
(deleted), patch
Paolo
: review+
Details | Diff | Splinter Review
https://tbpl.mozilla.org/php/getParsedLog.php?id=26741812&tree=Fx-Team Windows XP 32-bit fx-team debug test mochitest-browser-chrome on 2013-08-19 16:37:09 PDT for push 91ff451693ed slave: t-xp32-ix-041
Attached image Screenshot 10.6 (deleted) —
Any idea why this is more frequent on Mac?
Attached image Screenshot 10.7 (deleted) —
The failure point also looks different between 10.6 and 10.7. Too bad that browser-chrome tests have poor log diagnostics...
The problem here is because the method task_openPanel calls 'promiseFocus', to wait for the window to be focused. In a failing case, the popup opens while waiting for the window to be focused, so the test hangs later because it is waiting for a popup to open that is already open. In the working case, the popup opens later after it is being waited for. I'm not sure why the downloads panel is being opened early. Any ideas?
Attached patch basic_functionality_fix (obsolete) (deleted) — — Splinter Review
More details: The popup is opening earlier being a notification is occurring when the popup is added to the DownloadList and _notifyDownloadEvent calls showPopup. When the test fails, the popup is opened while waiting for focus before promisePanelOpened is called, so by the time promisePanelOpened is called the popup is already open so it never continues. We could just wait for focus earlier, although I'm not sure this will work. Really we should fix the popup handling so that is handles the case when the popup is already open. However when I tried that, I got errors stating that DownloadsPanel.panel was null.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #796627 - Flags: feedback?(paolo.mozmail)
(In reply to Neil Deakin from comment #78) > The popup is opening earlier being a notification is occurring when the > popup is added to the DownloadList and _notifyDownloadEvent calls showPopup. We could set the "browser.download.panel.shown" preference to disable this. By the way, many other unrelated tests start downloads in other areas of the code, and the first one that runs shows the Downloads Panel unnecessarily. The panel may also remain open while the following tests run. Is there a way to set the "browser.download.panel.shown" to true in a central place for all the mochitests? This seems a better behavior to me, even if different from the default release preference. > When the test fails, the popup is opened while waiting for focus before > promisePanelOpened is called, so by the time promisePanelOpened is called > the popup is already open so it never continues. For this, we may also update promisePanelOpened to return early and not wait for the event in case the popup is already open.
Comment on attachment 796627 [details] [diff] [review] basic_functionality_fix Review of attachment 796627 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/test/browser/head.js @@ +39,5 @@ > > //////////////////////////////////////////////////////////////////////////////// > //// Asynchronous support subroutines > > +function task_waitForFocus() The other functions starting with "task_" are generator functions, that must be called using Task.spawn or "yield". This is a function returning a promise instead, hence the different naming convention.
Attachment #796627 - Flags: feedback?(paolo.mozmail)
> Is there a way to set the "browser.download.panel.shown" to true in a central > place for all the mochitests? This seems a better behavior to me, even if > different from the default release preference. > Yes, we could set it in testing/profiles/prefs_general.js Note though that that preference is reset at http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/test/browser/head.js#79 The issue is having a test which wants to test what the preference is doing. I don't know if tests depend on this behaviour. > > When the test fails, the popup is opened while waiting for focus before > > promisePanelOpened is called, so by the time promisePanelOpened is called > > the popup is already open so it never continues. > > For this, we may also update promisePanelOpened to return early and not wait > for the event in case the popup is already open. I had assumed that a test that called promisePanelOpened expecting the panel to open then but instead have it already open might be a failure case for some tests.
Attached patch basic_functionality_fix (obsolete) (deleted) — — Splinter Review
If I run this test manually, I get an error saying that DownloadPanel.panel does not exist. I changed the panel getter to return null if the document's panel hasn't loaded yet.
Attachment #799610 - Flags: review?(paolo.mozmail)
Comment on attachment 799610 [details] [diff] [review] basic_functionality_fix Review of attachment 799610 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Neil Deakin from comment #132) > Yes, we could set it in testing/profiles/prefs_general.js We should do this! > Note though that that preference is reset at > http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/ > test/browser/head.js#79 > The issue is having a test which wants to test what the preference is doing. > I don't know if tests depend on this behaviour. The only test for which the preference is relevant is "browser_first_download_panel.js", so we can just set it to false there before starting, and clear it to the default (true for tests) in the finally block. > > > When the test fails, the popup is opened while waiting for focus before > > > promisePanelOpened is called, so by the time promisePanelOpened is called > > > the popup is already open so it never continues. > > > > For this, we may also update promisePanelOpened to return early and not wait > > for the event in case the popup is already open. > > I had assumed that a test that called promisePanelOpened expecting the panel > to open then but instead have it already open might be a failure case for > some tests. I agree with this view, but just ignoring this case seems to be more robust, from what we've seen... not sure if the new default preference would change the situation, though. ::: browser/components/downloads/content/downloads.js @@ +196,5 @@ > get panel() > { > + let downloadsPanel = document.getElementById("downloadsPanel"); > + if (!downloadsPanel) > + return null; nit: maybe we should add an explanatory comment. ::: browser/components/downloads/test/browser/head.js @@ +53,5 @@ > > + if (DownloadsPanel.panel && DownloadsPanel.panel.state == "open") { > + return deferred.resolve(); > + } > + nit: whitespace at end of line
Attachment #799610 - Flags: review?(paolo.mozmail)
Attached patch basic_functionality_fix (deleted) — — Splinter Review
Attachment #796627 - Attachment is obsolete: true
Attachment #799610 - Attachment is obsolete: true
Attachment #801162 - Flags: review?(paolo.mozmail)
Comment on attachment 801162 [details] [diff] [review] basic_functionality_fix Review of attachment 801162 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/content/downloads.js @@ +190,5 @@ > ////////////////////////////////////////////////////////////////////////////// > //// Panel interface > > /** > * Main panel element in the browser window. nit: "Main panel element in the browser window, or null if the panel overlay hasn't been loaded yet." ::: browser/components/downloads/test/browser/browser_first_download_panel.js @@ +53,5 @@ > } > } finally { > // Clean up when the test finishes. > yield task_resetState(); > + Services.prefs.setBoolPref("browser.download.panel.shown", oldPrefValue); nit: comment about why clearUserPref isn't appropriate here.
Attachment #801162 - Flags: review?(paolo.mozmail) → review+
https://hg.mozilla.org/mozilla-central/rev/9f7c4970b761
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(In reply to TBPL Robot from comment #177) > honzab.moz%firemni.cz > https://tbpl.mozilla.org/php/getParsedLog.php?id=28012847&tree=Gum On projects/gum where the fix has not been merged in yet.
(In reply to Honza Bambas (:mayhemer) from comment #178) > (In reply to TBPL Robot from comment #177) > > honzab.moz%firemni.cz > > https://tbpl.mozilla.org/php/getParsedLog.php?id=28012847&tree=Gum > > On projects/gum where the fix has not been merged in yet. hm seems to be still at fx-team like https://tbpl.mozilla.org/php/getParsedLog.php?id=31844828&tree=Fx-Team so reopen ?
Flags: needinfo?(honzab.moz)
No idea and not sure why you are asking me :)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(enndeakin)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Intermittent chrome://mochitests/content/browser/browser/components/downloads/test/browser/browser_basic_functionality.js | Test timed out → Intermittent downloads/test/browser/browser_basic_functionality.js | Test timed out
This doesn't seem to be a problem any more.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Flags: needinfo?(enndeakin)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: