Closed Bug 1130411 Opened 10 years ago Closed 9 years ago

Intermittent browser_change_app_handler.js | Test timed out | Found a tab

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Platform:
x86_64
Windows 8.1
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 39
Tracking Flags:
Tracking Status
firefox37 --- unaffected
firefox38 --- fixed
firefox39 --- fixed
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: RyanVM, Assigned: Gijs)

References

Details

(Keywords: intermittent-failure)

Attachments

(3 files)

use command event for browser_change_app_handler.js in effort to reduce intermittent orange,
2.52 KB, patch
jaws
: review+
Gijs
: checkin+
Details | Diff | Splinter Review
only care about unload if it involves the page we opened,
2.02 KB, patch
jaws
: review+
lsblakk
: approval-mozilla-aurora+
Gijs
: checkin+
Details | Diff | Splinter Review
MozReview Request: Bug 1130411 - deal with opening dialogs when the previous dialog is not unloaded yet, r?jaws
58 bytes, text/x-review-board-request
jaws
: review+
ritu
: approval-mozilla-aurora+
Details
18:19:57 INFO - 1400 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | All expectedStyleSheetURLs should have been found 18:19:57 INFO - 1401 INFO Dialog loaded 18:19:57 INFO - 1402 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | App should be set as preferred. 18:19:57 INFO - 1403 INFO Got list after item was selected 18:19:57 INFO - 1404 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | Should have a selected item 18:19:57 INFO - 1405 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | App should be visible as preferred item. 18:19:57 INFO - 1406 INFO Console message: [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITaskbarTabPreview.invalidate]" {file: "resource://app/modules/WindowsPreviewPerTab.jsm" line: 401}] 18:19:57 INFO - 1407 INFO Console message: [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITaskbarTabPreview.invalidate]" {file: "resource://app/modules/WindowsPreviewPerTab.jsm" line: 401}] 18:19:57 INFO - 1408 INFO Console message: 1423189149865 Services.HealthReport.HealthReporter WARN Saved state file does not exist. 18:19:57 INFO - 1409 INFO TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_change_app_handler.js | Test timed out - expected PASS 18:19:57 INFO - 1410 INFO TEST-FAIL | browser/components/preferences/in-content/tests/browser_change_app_handler.js | Assertion count 1 is greater than expected range 0-0 assertions. 18:19:57 INFO - 1411 INFO MEMORY STAT vsize after test: 840101888 18:19:57 INFO - 1412 INFO MEMORY STAT vsizeMaxContiguous after test: 8297398345728 18:19:57 INFO - 1413 INFO MEMORY STAT residentFast after test: 331448320 18:19:57 INFO - 1414 INFO MEMORY STAT heapAllocated after test: 57568536 18:19:57 INFO - 1415 INFO TEST-OK | browser/components/preferences/in-content/tests/browser_change_app_handler.js | took 90359ms 18:19:57 INFO - 1416 INFO TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_change_app_handler.js | Found a tab after previous test timed out: about:preferences#applications - expected PASS
Flags: needinfo?(jaws)
I thought this was fixed by backing out bug 1044600. https://bugzilla.mozilla.org/show_bug.cgi?id=1044600#c9
Flags: needinfo?(jaws) → needinfo?(ryanvm)
I thought so too, but this was on inbound and AFAIK, bug 1044600 never got past fx-team.
Flags: needinfo?(ryanvm)
I wonder if the test I landed is just racy or something. :-( (although it's odd that it would only start showing up this long after it landed... anything else this correlates with?)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #8) > I wonder if the test I landed is just racy or something. :-( The test I landed did so on Jan.26. If it first showed problems on Feb 3 / 6, that's an awfully long time (considering frequency in this bug and when bug 1044600 landed). So I guess something else impacted this, but goodness knows what. :-\ > > (although it's odd that it would only start showing up this long after it > landed... anything else this correlates with?) I don't see anything in http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2015-02-06&enddate=2015-02-07 that seems reasonably related. :-\
Flags: needinfo?(gijskruitbosch+bugs)
This might seem like a dumb/avoiding-responsibility question, but these are all Win8 ix machines running debug, and the dialog we're trying to show here (and end up waiting forever for) depends on system apps. Did anything change wrt the configuration of these machines?
Flags: needinfo?(ryanvm)
Not that I'm aware of.
Flags: needinfo?(ryanvm)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Egh, did not mean to take this. Anyway, reasoning being, if the click is fizzling because layout/xul decides not to turn it into a command event which the event listeners pick up, this would fix it. The next step would be adding logging to the dialog code to see if it's even being hit in these cases (and if so, why there's no visual evidence of the same)
Assignee: gijskruitbosch+bugs → nobody
Comment on attachment 8561420 [details] [diff] [review] use command event for browser_change_app_handler.js in effort to reduce intermittent orange, Review of attachment 8561420 [details] [diff] [review]: ----------------------------------------------------------------- It looks like this might fix the intermittent orange I was hitting in bug 1044600. I pushed this patch and the patch for bug 1044600 to tryserver to see if it's now good to land: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c95bd3b59730
Attachment #8561420 - Flags: review?(jaws) → review+
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14) > Comment on attachment 8561420 [details] [diff] [review] > use command event for browser_change_app_handler.js in effort to reduce > intermittent orange, > > Review of attachment 8561420 [details] [diff] [review]: > ----------------------------------------------------------------- > > It looks like this might fix the intermittent orange I was hitting in bug > 1044600. I pushed this patch and the patch for bug 1044600 to tryserver to > see if it's now good to land: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c95bd3b59730 This doesn't seem to be helping, judging by that try run... :-\
I landed this anyway because I don't want to be playing whack-a-mole with the intermittent, and this seems like a strict improvement: https://hg.mozilla.org/integration/fx-team/rev/72dc34c5574f That said, haven't seen this since early on Monday (late on Sunday US time, I guess), which makes me wonder if it was related to something else... Going to go and redo the trypush from comment #14 and include some logging in subdialogs.js (not fit for permanent checkin) to see if we can figure out what's going on.
Status: ASSIGNED → NEW
Keywords: leave-open
Attachment #8561420 - Flags: checkin+
Recurring things from today/yesterday were from the attempted relanding of bug 1044600. Let's close this out.
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: leave-open
Resolution: --- → WORKSFORME
Blocks: 1044600
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
While I gather that this is appearing in debug/testing machines, I'm seeing tons of this type error in the Browser Console on Nightly builds, and its been occurring for some time. 19:03:25.140 NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITaskbarTabPreview.invalidate]1 WindowsPreviewPerTab.jsm:406:0 Seems to trigger the error switching between browser windows, or tabs, but not all the time. i.e. not 100% reproducible. There could be a timing function maybe before it re-triggers..
Oh, sorry for spam, forgot to add this on Win7 x64
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #100) > While I gather that this is appearing in debug/testing machines, I'm seeing > tons of this type error in the Browser Console on Nightly builds, and its > been occurring for some time. > > 19:03:25.140 NS_ERROR_FAILURE: Component returned failure code: 0x80004005 > (NS_ERROR_FAILURE) [nsITaskbarTabPreview.invalidate]1 > WindowsPreviewPerTab.jsm:406:0 > > Seems to trigger the error switching between browser windows, or tabs, but > not all the time. i.e. not 100% reproducible. There could be a timing > function maybe before it re-triggers.. Uh, this has nothing to do with this test or its intermittent failures. It happens to show up in the log. Please file a new bug (and/or search for one that is more topical).
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → WORKSFORME
Harumph.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
From the log: 08:30:45 INFO - Failed to load jar:file:///C:/slave/test/build/application/firefox/browser/omni.ja!/chrome/browser/content/browser/preferences/applicationManager.js Err, that doesn't sound good. :-\
(In reply to :Gijs Kruitbosch from comment #110) > From the log: > > 08:30:45 INFO - Failed to load > jar:file:///C:/slave/test/build/application/firefox/browser/omni.ja!/chrome/ > browser/content/browser/preferences/applicationManager.js > > Err, that doesn't sound good. :-\ I don't see this on any successful logs, though those do show the same assertions. I wonder why this script load fails, and/or how we can debug that, and/or how we can check it has anything to do with the test failure (because the test failure messages are disjoint from the assertion/debug spew that is generated during the test, helpfully... Chris, is there some way to get a unified log with the right timestamps? At the current rate, I doubt that I'll be able to repro locally to figure this out. :-(
Flags: needinfo?(cmanchester)
SimpleTest.requestCompleteLog() will prevent test output suppression/buffering for the course of a test. I noticed this assertion and a stack in some recent debug failure logs above the "Failed to load" line that might be relevant, although I guess you probably already looked into it: [1780] ###!!! ASSERTION: |First()| called on an empty string: 'mLength > 0', file c:\builds\moz2_slave\fx-team-w32-d-0000000000000000\build\src\obj-firefox\dist\include\nsTSubstring.h, line 273
Flags: needinfo?(cmanchester)
(In reply to Chris Manchester [:chmanchester] from comment #112) > SimpleTest.requestCompleteLog() will prevent test output > suppression/buffering for the course of a test. I noticed this assertion and > a stack in some recent debug failure logs above the "Failed to load" line > that might be relevant, although I guess you probably already looked into it: > > [1780] ###!!! ASSERTION: |First()| called on an empty string: 'mLength > 0', > file > c:\builds\moz2_slave\fx-team-w32-d-0000000000000000\build\src\obj- > firefox\dist\include\nsTSubstring.h, line 273 Of course, that assertion is in the passing log. At any rate, let me know if this doesn't work as expected or there's anything I can do to help.
Turned on the complete logging per comment #112 - thanks! https://hg.mozilla.org/integration/fx-team/rev/760b6569fd2f
Keywords: leave-open
Comment 130 has the extra logging turned on. It has "TEST-PASS"s, then the message from comment 110, then the passage of time, then the timeout message.
(In reply to Chris Manchester [:chmanchester] from comment #133) > Comment 130 has the extra logging turned on. It has "TEST-PASS"s, then the > message from comment 110, then the passage of time, then the timeout message. Yeah... Jared, I still suspect that the setTimeout'd unload thing has a hand in this. The window gets destroyed pointing to applicationManager.xul (which is the second URL we use). Specifically, I think this sequence is happening: 1) we open the first URL 2) we close the dialog by clicking OK this dispatches the dialogclosing event, which we keep and store, and then calls window.close . this will call gSubDialog.close() this removes all our listeners (!) and indicates we're closing this does a setTimeout with a timeout value of 0 that will cause about:blank to be loaded instead of the current document; then we have this yield: list = yield waitForCondition(() => win.document.getAnonymousElementByAttribute(ourItem, "class", "actionsMenu")); which AIUI should wait at least 100ms (which is assuming timeouts are deterministic, which we know they're not). 3) we open the second URL... and readd all the listeners in one go. I can think of two possible things which explain the log from comment 130: a) the unload somehow races with the condition and while we first open with the second URL, then the loadURI call gets run and we replace it with about:blank, which closes all the things through the unload handler that we attach, and kills the load of the JS file as well. b) some unknown thing kills the JS file which somehow causes the load of the document to never fire (before that, because of our opacity hacks, you won't see the new dialog that *has* been opened, AFAICT...) I've tried to verify (a) with the try push in comment #128, but it seems win7 debug isn't being run for whatever reason (maybe I'll be lucky when I wake up tomorrow morning?). Jared, thoughts? At least for (a), it seems to me we could avoid it ever happening by checking in the function we setTimeout whether a new load is already pending? Is that crazy talk, and/or can you think of something better, and/or do you think there's anything wrong with my reasoning and/or can you think of any reasons besides (a) and (b) ? :-)
Flags: needinfo?(jaws)
So the try push logs the following: 21:06:10 INFO - 1397 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | All expectedStyleSheetURLs should have been found 21:06:10 INFO - 1398 INFO Dialog loaded 21:06:10 INFO - 1399 INFO Dialog loaded (two logs because the test itself logs this, and gSubdialogs logs this in my try push) 21:06:10 INFO - 1400 INFO gSubdialogs.close called, event type: dialogclosing isClosing: false So at this point, appPicker.xul is being killed off because we clicked OK. We now remove all the event listeners. 21:06:10 INFO - 1401 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | App should be set as preferred. This is still sync and we check the internal state is correct 21:06:10 INFO - 1402 INFO Loading about:blank This happens on the setTimeout from the close, above. We've now called loadURI with about:blank. 21:06:10 INFO - 1403 INFO Got list after item was selected 21:06:10 INFO - 1404 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | Should have a selected item 21:06:10 INFO - 1405 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | App should be visible as preferred item. And all of this happens in the the setTimeout with delay 100ms (plus any async-ness introduced by yield and promises being resolved), when we've checked that stuff has been updated in the UI. 21:06:10 INFO - 1406 INFO Opening dialog with chrome://browser/content/preferences/applicationManager.xul 21:06:10 INFO - 1407 INFO Reset isClosing and closingEvent to false and null, respectively Great, we're moving on and showing the next dialog. This re-adds event listeners. 21:06:10 INFO - 1408 INFO unload fired for chrome://global/content/appPicker.xul Wait what? So, three options that I can see: 1) somehow, the loadURI of about:blank no-ops (and we only kill off the old thing now, when we load applicationManager.xul) 2) the unload caused by the loadURI of about:blank is delayed/async and only fires now 3) ... something else causes unload to fire? I have no idea what that would be, and why it'd have this window target (the URL is nicked from aEvent.target.location.href) 21:06:10 INFO - 1409 INFO gSubdialogs.close called, event type: (none) isClosing: false The unload event handler calls .close(). The event not being present isn't all that meaningful: it could be that we've not attached our dialogclosing handlers, because we only do that onContentLoaded, and I didn't log from there (ugh). If the handler isn't there, it could still be that it was the dialog's own methods or whatever that were (somehow) invoked and unloaded the document. 21:06:10 INFO - ++DOMWINDOW == 105 (12D4E400) [pid = 3288] [serial = 109] [outer = 1D446A00] 21:06:10 INFO - 1410 INFO Loading about:blank And this is the second time we're trying to load about:blank, because gSubdialogs.close will have set another timeout to do so. 21:06:10 INFO - Failed to load jar:file:///C:/slave/test/build/application/firefox/browser/omni.ja!/chrome/browser/content/browser/preferences/applicationManager.js 21:06:10 INFO - ++DOMWINDOW == 106 (12D4E900) [pid = 3288] [serial = 110] [outer = 1D446A00] 21:06:10 INFO - [3288] WARNING: NS_ENSURE_TRUE(mScriptGlobalObject) failed: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\dom\xul\XULDocument.cpp, line 3555 21:06:11 INFO - [3288] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\docshell\base\nsDocShell.cpp, line 4567 21:06:11 INFO - [3288] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\docshell\base\nsDocShell.cpp, line 4567 docshell seems to be confused about it not having a script global object etc. for the window which is now going away, but I have no idea if that's useful info at all.
For comparison, a passing log shows: 21:04:27 INFO - 1397 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | All expectedStyleSheetURLs should have been found 21:04:27 INFO - 1398 INFO Dialog loaded 21:04:27 INFO - 1399 INFO Dialog loaded 21:04:27 INFO - 1400 INFO gSubdialogs.close called, event type: dialogclosing isClosing: false 21:04:27 INFO - 1401 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | App should be set as preferred. 21:04:27 INFO - 1402 INFO Loading about:blank 21:04:27 INFO - ++DOMWINDOW == 109 (1F132D80) [pid = 3948] [serial = 109] [outer = 0ECB7500] 21:04:27 INFO - 1403 INFO Got list after item was selected 21:04:27 INFO - 1404 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | Should have a selected item 21:04:27 INFO - 1405 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | App should be visible as preferred item. 21:04:27 INFO - 1406 INFO Opening dialog with chrome://browser/content/preferences/applicationManager.xul 21:04:27 INFO - 1407 INFO Reset isClosing and closingEvent to false and null, respectively 21:04:27 INFO - 1408 INFO unload fired for about:blank 21:04:27 INFO - ++DOMWINDOW == 110 (1F1EEC80) [pid = 3948] [serial = 110] [outer = 0ECB7500] 21:04:27 INFO - [3948] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\layout\style\Loader.cpp, line 2090 21:04:27 INFO - JavaScript strict warning: chrome://browser/content/preferences/applications.js, line 1214: ReferenceError: reference to undefined property this._visibleTypeDescriptionCount[aHandlerInfo.description] 21:04:27 INFO - 1409 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | Check the proper URL is loaded 21:04:27 INFO - 1410 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | Element should not be null, when checking visibility 21:04:27 INFO - 1411 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | Overlay is visible 21:04:27 INFO - 1412 INFO found chrome://mozapps/content/preferences/preferences.css 21:04:27 INFO - 1413 INFO found chrome://browser/skin/preferences/preferences.css 21:04:27 INFO - 1414 INFO found chrome://global/skin/in-content/common.css 21:04:27 INFO - 1415 INFO found chrome://browser/skin/preferences/in-content/preferences.css 21:04:27 INFO - 1416 INFO found chrome://browser/skin/preferences/in-content/dialog.css 21:04:27 INFO - 1417 INFO TEST-PASS | browser/components/preferences/in-content/tests/browser_change_app_handler.js | All expectedStyleSheetURLs should have been found 21:04:27 INFO - 1418 INFO Dialog loaded 21:04:27 INFO - 1419 INFO Dialog loaded the second time so it seems that here, when we load the new file, about:blank has loaded in the frame by now and that gets unloaded (and we correctly ignore that happening). In any case, it seems that at least part of the issue is that the unload handler is firing when it shouldn't, and is calling .close . We could see if attaching the unload handler only after the dialog has loaded works? Is there some reason that'd be a bad idea?
(In reply to :Gijs Kruitbosch from comment #137) > We could see if attaching the unload handler only after the dialog has > loaded works? Is there some reason that'd be a bad idea? Yeah that sounds like something we should try. Also see if we can get the same behavior either without the setTimeout or having something in the setTimeout bail early if a condition isn't met (like your check for a new load mentioned above).
Flags: needinfo?(jaws)
Wrong bug number, but remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c02925ad34a6 Let's hope this fixes things.
Attachment #8577924 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
Attachment #8577924 - Flags: review?(jaws) → review+
remote: https://hg.mozilla.org/integration/fx-team/rev/cdf8e5b44c56 Nuking leave-open because this should fix stuff...
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/cdf8e5b44c56
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8577924 [details] [diff] [review] only care about unload if it involves the page we opened, Approval Request Comment [Feature/regressing bug #]: in-content-pref dialogs [User impact if declined]: intermittent orange, but also, theoretical issues with dialogs not opening in release builds if machines are very slow. [Describe test coverage new/current, TreeHerder]: see intermittent oranges... [Risks and why]: very low, icp dialogs have decent test coverage at this point, so if there was anything very strange going on, the tests would have caught it. [String/UUID change made/needed]: nope
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8577924 - Flags: approval-mozilla-aurora?
Hi Gijs, can you provide a point value.
Iteration: --- → 39.2 - 23 Mar
Flags: qe-verify?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: firefox-backlog+
Points: --- → 5
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8577924 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite+
Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: gijskruitbosch+bugs → nobody
Iteration: 39.2 - 23 Mar → ---
Points: 5 → ---
Attachment #8577924 - Flags: checkin+
Flags: needinfo?(gijskruitbosch+bugs)
I looked at this last week and could intermittently reproduce with a downloaded debug build, and then that went away again. Still no idea why this is happening. We seem to be getting stuck trying to get the "manage" subdialog to open, but alternative ways of opening that dialog didn't really get me conclusive evidence about what was going wrong... I'll try to make time to look at this later this week or, failing that, next week, as I'm aware it's one of the top oranges. :-\
Actually, re-reading some of the code here... I wonder if the issue is that we're not waiting for the about:blank load in the subdialog, and the setTimeout of the first dialog fires after we try opening the second dialog, breaking the load of that dialog. In which case, I think that's technically a real bug in our subdialogs code, though you'd be hard-pushed to run into it in practice (we seem to only be hitting this in debug mode...) . I trypushed with a bunch of logging to see if I can verify this theory: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7257f340df01 In the meantime, Jared, does that seem at all plausible?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jaws)
(In reply to :Gijs Kruitbosch from comment #229) > Actually, re-reading some of the code here... I wonder if the issue is that > we're not waiting for the about:blank load in the subdialog, and the > setTimeout of the first dialog fires after we try opening the second dialog, > breaking the load of that dialog. > > In which case, I think that's technically a real bug in our subdialogs code, > though you'd be hard-pushed to run into it in practice (we seem to only be > hitting this in debug mode...) . > > I trypushed with a bunch of logging to see if I can verify this theory: > > remote: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7257f340df01 > > In the meantime, Jared, does that seem at all plausible? The trypush says I'm right, which is always reassuring. Now I just need to come up with a fix...
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
Flags: needinfo?(jaws)
Issues that affect debug mode can also be seen by people running Firefox in a very slow environment (concurrent virus scan, poor harddrive speeds, memory paging). I guess that's plausible??? But it's kinda weird because it's assuming that the setTimeout takes a while to run but that we're moving through the test faster than the setTimeout? Can we remove the setTimeout and just load about:blank from the close() function? Is the event handler for onKeyDown/ESC not able to prevent the load-stop of about:blank? What if we just created a new subdialog each time a subdialog is opened, thus making closing it a lot simpler and removing state that has to be cleaned up between showings?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #232) > Issues that affect debug mode can also be seen by people running Firefox in > a very slow environment (concurrent virus scan, poor harddrive speeds, > memory paging). > > I guess that's plausible??? But it's kinda weird because it's assuming that > the setTimeout takes a while to run but that we're moving through the test > faster than the setTimeout? Can we remove the setTimeout and just load > about:blank from the close() function? Is the event handler for > onKeyDown/ESC not able to prevent the load-stop of about:blank? I mean, the assertion from the code there, which has been there since Matt wrote this originally, is that if we don't do it on a timeout, closing a subdialog with the esc key would bust things because it stops the about:blank load and we keep the dialog loaded. > What if we just created a new subdialog each time a subdialog is opened, > thus making closing it a lot simpler and removing state that has to be > cleaned up between showings? This could kind of work but we currently have a gSubDialog thing whose ._frame always refers to the same DOM element, and we're always using the dialogFrame element instead of a new element each time. Breaking some of those invariants seems scary to me... I noticed that the open() call returns a reference to the window we've just opened, but no code (not even add-ons!) seems to depend on this right now. So I have a patch that just makes open() deal with a different thing still being loaded / unloaded, and waiting for that to complete (including the about:blank load on a timeout). That seems like a smaller and thereby safer change... It makes open() async, but it already kind of is, apart from that return value, so that seems OK. That said, I'd like to write specific tests for some of this, and that's why no patch is up yet, sorry.
Comment on attachment 8737536 [details] MozReview Request: Bug 1130411 - deal with opening dialogs when the previous dialog is not unloaded yet, r?jaws https://reviewboard.mozilla.org/r/44003/#review40571 +1 for the test
Attachment #8737536 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/404719a4fd5b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 9 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Comment on attachment 8737536 [details] MozReview Request: Bug 1130411 - deal with opening dialogs when the previous dialog is not unloaded yet, r?jaws Approval Request Comment [Feature/regressing bug #]: n/a [User impact if declined]: opening subdialogs in about:preferences is a bit unreliable, high-frequency intermittents result [Describe test coverage new/current, TreeHerder]: comes with tests, fixes intermittent orange! [Risks and why]: fairly low because of the test coverage [String/UUID change made/needed]: nope NB: the intermittent affects beta but I would prefer not to crash-land this into beta as late as it is. Can't land with test-only approval because this commit actually also changes application code.
Attachment #8737536 - Flags: approval-mozilla-aurora?
Comment on attachment 8737536 [details] MozReview Request: Bug 1130411 - deal with opening dialogs when the previous dialog is not unloaded yet, r?jaws Fixes an intermittent failure and has automated test coverage, Aurora47+
Attachment #8737536 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1312216
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: