Closed Bug 1192800 Opened 10 years ago Closed 8 years ago

Intermittent browser_context_menu.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW

Categories

(Toolkit :: Password Manager, defect, P5)

Product:
Toolkit
Component:
Password Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
e10s + ---
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: RyanVM, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1192800 - Get rid of CPOWs in browser_context_menu.js.
59 bytes, text/x-review-board-request
MattN
: review+
Details
19:34:45 INFO - 841 INFO checking window state 19:34:45 INFO - 842 INFO Entering test test_initialize 19:34:45 INFO - 843 INFO Leaving test test_initialize 19:34:45 INFO - 844 INFO Entering test test_context_menu_populate 19:34:45 INFO - 845 INFO TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_context_menu.js | Same amount of menu items and expected logins. - 2 == 2 19:34:45 INFO - 846 INFO TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_context_menu.js | Every login have an item at the menu. - true == true 19:34:45 INFO - 847 INFO Leaving test test_context_menu_populate 19:34:45 INFO - 848 INFO Entering test test_context_menu_password_fill 19:34:45 INFO - 849 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_context_menu.js | Uncaught exception - at :0 - Error: operation not possible on dead CPOW 19:34:45 INFO - Stack trace: 19:34:45 INFO - test_context_menu_password_fill@chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_context_menu.js:95:9 19:34:45 INFO - test_context_menu_password_fill@chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_context_menu.js:95:9 19:34:45 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23 19:34:45 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7 19:34:45 INFO - this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:1 19:34:45 INFO - promise callback*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:9 19:34:45 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:776:7 19:34:45 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:711:7 19:34:45 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:745:9 19:34:45 INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:668:7 19:34:45 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:745:59 19:34:45 INFO - 850 INFO Leaving test test_context_menu_password_fill 19:34:45 INFO - 851 INFO Entering test test_context_menu_iframe_fill 19:34:46 INFO - 852 INFO TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_context_menu.js | Password filled and correct. - "password-cross-origin" == "password-cross-origin" 19:34:46 INFO - 853 INFO TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_context_menu.js | Username value was not changed. - "" == "" 19:34:46 INFO - 854 INFO Leaving test test_context_menu_iframe_fill 19:34:46 INFO - MEMORY STAT | vsize 20972906MB | residentFast 510MB 19:34:46 INFO - 855 INFO TEST-OK | toolkit/components/passwordmgr/test/browser/browser_context_menu.js | took 9768ms 19:34:46 INFO - Not taking screenshot here: see the one that was previously logged 19:34:46 INFO - 856 INFO TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_context_menu.js | Found an unexpected tab at the end of test run: https://example.com/browser/toolkit/components/passwordmgr/test/browser/multiple_forms.html - expected PASS
Blocks: 433238
Hey Matt, this seems to happen very frequently now, can you take a look there? Thanks!
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo) → needinfo?(bernardo)
Yesterday's change in chunking both made it pretty much permaorange, and added in the fun of intermittently crashing instead of failing, so I disabled it on Linux. By copy-pasting the line disabling the previous test in the .ini on Linux, so I'm looking forward to seeing whether we will now have to disable the next test, since "failed after a change in chunking" can sometimes mean that the failing test itself depends on having some previous test run, and that test got moved out of the same chunk, but it can also mean that it happens because some previous test is broken and leaves some of its brokenness around to break a later test, in which case disabling just moves the broken down to the next vulnerable one.
Whiteboard: [test disabled on Linux]
I think when I reviewed this test I thought that withNewTab ran the task function as a content task but it's not so this file is actually use CPOWs… I think that would be okay (but not ideal) but I guess the intermittent failures come from the tab being opened but the content isn't yet. Disabling for e10s: https://hg.mozilla.org/integration/fx-team/rev/034378ebabfc
Keywords: leave-open
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [test disabled on Linux] → [test disabled for e10s]
Intermittent e10s test failure
Priority: -- → P5
Re-trigger "browser_context_menu.js" test for multiple times[1] to reproduce the intermittent failure. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=64e10772086a
Pushed a new try[1] to test the timing issue. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66680a3a8fc6
Pushed a new try[1] to enable the test in e10s environment and test the timing issue. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dbb79607a92
Triggered "browser_context_menu.js" test for 50 times to test the timing issue. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dbb79607a92
Pushed a new try[1]: Wait for browser window is loaded and test the timing issue. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86b451a28c3a
Retriggered "browser_context_menu.js" test for 50 times to test the timing issue. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86b451a28c3a&selectedJob=27621736
Through some weird circumstances this is blocking our progress in bug 1337772 (by failing our try push) and that means I'll try to remove those CPOWs now to either fix the issue or get a better understanding what's wrong there.
Assignee: nobody → jhofmann
Blocks: 1337772
Status: NEW → ASSIGNED
It's not incredibly pretty right now, and it relies on the fact that each form has a unique description. We can also start giving forms unique ids, maybe that's a bit more resilient. There's obviously a lot of shared variables between content and browser code.
Comment on attachment 8847997 [details] Bug 1192800 - Get rid of CPOWs in browser_context_menu.js. https://reviewboard.mozilla.org/r/120952/#review123136 This would have been easier to review as multiple commits… (especially the openPasswordContextMenu change in its own). Giving up for now due to the few issues found already. ::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:123 (Diff revision 2) > Services.prefs.setBoolPref("signon.schemeUpgrades", true); > yield BrowserTestUtils.withNewTab({ > gBrowser, > url: TEST_HOSTNAME + MULTIPLE_FORMS_PAGE_PATH, > }, function* (browser) { > + let contextMenu = document.getElementById("contentAreaContextMenu"); Nit: You could have defined this as a `const` (with appropriate uppercase/underscore style) instead of locally. ::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:130 (Diff revision 2) > + let formDescriptions = yield ContentTask.spawn(browser, {}, function*() { > + let forms = Array.from(content.document.getElementsByClassName("test-form")); > + return forms.map((f) => f.getAttribute("description")); > + }); > > - let testForms = browser.contentWindow.document.getElementsByClassName("test-form"); > + for (let form of formDescriptions) { Nit: Rename `form` to `description` or `formDescription` since it's not a form ::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:133 (Diff revision 2) > - let testForms = browser.contentWindow.document.getElementsByClassName("test-form"); > - for (let form of testForms) { > + for (let form of formDescriptions) { > + info("Testing form: " + form); > - let usernameInputList = form.querySelectorAll("input[type='password']"); > - info("Testing form: " + form.getAttribute("description")); > > - for (let passwordField of usernameInputList) { > + let passwordInputIds = yield ContentTask.spawn(browser, {form}, function*({form}) { Same for this argument ::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:134 (Diff revision 2) > - info("Testing password field: " + passwordField.id); > + let formElement = content.document.querySelector(`[description="${form}"]`); > + let passwords = Array.from(formElement.querySelectorAll("input[type='password']")); Nit: You could have probably used one querySelectorAll and did this in one line but it's okay as-is ::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:212 (Diff revision 2) > + let hidden = popupHeader.hidden; > + let disabled = popupHeader.disabled; IMO these are bad variable names since this same fiele deals with the disabled property on inputs too. `headerHidden`/`headerDisabled` would be clearer. ::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:256 (Diff revision 2) > - let unchangedFields = form.querySelectorAll("input:not(#" + usernameField.id + "):not(#" + passwordField.id + ")"); > - yield assertContextMenuFill(form, usernameField, passwordField, unchangedFields, 1); > + yield assertContextMenuFill(browser, form, inputId, passwordFieldId, 1); > + IIUC, the new implementation of `assertContextMenuFill` doesn't handle `usernameField.id`. IMO it would have been better to leave that argument to reduce the churn and keep the flexibility.
Attachment #8847997 - Flags: review?(MattN+bmo)
>This would have been easier to review as multiple commits… (especially the openPasswordContextMenu change in its own). Sorry for that, I didn't really expect the change to become as large as it did and so never considered splitting it up. > Nit: You could have probably used one querySelectorAll and did this in one line but it's okay as-is Yeah, I had the same thought, but it would have gotten so long that I would have had to break lines anyway. > IIUC, the new implementation of `assertContextMenuFill` doesn't handle `usernameField.id`. IMO it would have been better to leave that argument to reduce the churn and keep the flexibility. Good catch!
Comment on attachment 8847997 [details] Bug 1192800 - Get rid of CPOWs in browser_context_menu.js. https://reviewboard.mozilla.org/r/120952/#review126552 ::: toolkit/components/passwordmgr/test/browser/browser_context_menu.js:5 (Diff revision 3) > /* > * Test the password manager context menu. > */ > > +/* eslint no-shadow:"off" */ I would have preferred we didn't shadow but okay
Attachment #8847997 - Flags: review?(MattN+bmo) → review+
Thanks :)
Flags: needinfo?(bernardo)
Keywords: leave-open
Whiteboard: [test disabled for e10s]
Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/329a623e556d Get rid of CPOWs in browser_context_menu.js. r=MattN
https://hg.mozilla.org/mozilla-central/rev/329a623e556d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: