Closed Bug 1402555 Opened 8 years ago Closed 8 years ago

deleting history from urlbar completion list doesn't work

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Version:
57 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 + fixed
firefox58 --- fixed

People

(Reporter: lilydjwg, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1402555 - deleting history from urlbar completion list doesn't work.
59 bytes, text/x-review-board-request
adw
: review+
ritu
: approval-mozilla-beta+
Details
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20170921100141 Steps to reproduce: type something in the urlbar. A completion list will show. Delete some history items by selecting and pressing Delete. Actual results: Although the items are removed from the UI, the history items are still there. I can redo the completion to see them again. Expected results: The history items are no longer there, like if I delete them from the history side panel. This worked for me for a couple of years until recently I notice that it doesn't work any more.
Status: UNCONFIRMED → NEW
Has STR: --- → yes
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
Component: Untriaged → Address Bar
Ever confirmed: true
Keywords: regression
Priority: -- → P1
Whiteboard: [fxsearch]
tracking as new regression in 57
tracking-firefox57: ? → +
Assignee: nobody → mak77
Status: NEW → ASSIGNED
See Also: → 1404429
The bug is due to the removal of the listener from the result at the end of the search. I got a patch, but it needs a test.
See Also: 1404429 →
Hmm, is it OK that these new listeners are never removed? (1) Will they leak? Or is that not a problem because they'll go away once results go away? (2) Can we get into a situation where multiple new listeners are added to the same result (via previousResult)? To me it seems like the simpler and safer fix would be to keep the UnifiedComplete singleton as the listener, but never remove it as the listener. IOW to only remove the `this._result.setListener(null);` line -- and also make sure UnifiedComplete doesn't keep a reference (direct or indirect) to the result after the result isn't needed anymore, using weak reference(s) if necessary, so that there's no cycle. But I haven't thought about it as deeply as you probably have.
(In reply to Drew Willcoxon :adw from comment #9) > Hmm, is it OK that these new listeners are never removed? (1) Will they > leak? Or is that not a problem because they'll go away once results go > away? (2) Can we get into a situation where multiple new listeners are > added to the same result (via previousResult)? The listener is kept alive by the simple result mListener property and it goes away with the result. Setting a new listener removes the old one, since there only 1 mListener property. You can only have one listener at a time. > To me it seems like the simpler and safer fix would be to keep the > UnifiedComplete singleton as the listener, but never remove it as the > listener. IOW to only remove the `this._result.setListener(null);` line -- > and also make sure UnifiedComplete doesn't keep a reference (direct or > indirect) to the result after the result isn't needed anymore, using weak > reference(s) if necessary, so that there's no cycle. But I haven't thought > about it as deeply as you probably have. Yes the problem is cycles, it's very easy to create cycles here. The only points where the previous result is no more needed is when we figure out we won't use it (the next search), but if the user doesn't run a new search, the result will make everything connected leak on shutdown. Then we'd need either to implement cycle collecting in the autocomplete controller, or add a shutdown blocker. Both options sound far more complex than what I did, and that's the main reason I went this path. The patch is trivial and good for an uplift. Fwiw, the only reason we need to keep a result around in unified complete is that the controller doesn't pass anymore the previous result in case of backspace. Maybe it should, but I'm a bit scared of regressions in other consumers. surely not something I'd try in a patch for uplift, we could examine that at the beginning of a cycle, and some complication may go away. A weak reference may work, but which kind of weak reference did you have in mind? A WeakSet sounds like excessive a single entry, even if potentially it may work.
Comment on attachment 8915723 [details] Bug 1402555 - deleting history from urlbar completion list doesn't work. https://reviewboard.mozilla.org/r/186928/#review192338 OK, thanks for explaining. "setListener" does imply there's only one listener at a time, sorry for not realizing that.
Attachment #8915723 - Flags: review?(adw) → review+
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/3faf2c49b172 deleting history from urlbar completion list doesn't work. r=adw
Backed out for failing xpcshell's toolkit/components/places/tests/unifiedcomplete/test_autocomplete_on_value_removed_479089.js: https://hg.mozilla.org/integration/autoland/rev/5159cb2f4b419a45386cb5cf76005f42cdba88e0 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3faf2c49b17295fc1d6ffa1696048b9db672c264&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135495370&repo=autoland [task 2017-10-07T10:39:23.626Z] 10:39:23 INFO - TEST-START | toolkit/components/places/tests/unifiedcomplete/test_autocomplete_on_value_removed_479089.js [task 2017-10-07T10:39:26.212Z] 10:39:26 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/places/tests/unifiedcomplete/test_autocomplete_on_value_removed_479089.js | xpcshell return code: 0 [task 2017-10-07T10:39:26.221Z] 10:39:26 INFO - TEST-INFO took 2585ms [task 2017-10-07T10:39:26.223Z] 10:39:26 INFO - >>>>>>> [task 2017-10-07T10:39:26.224Z] 10:39:26 INFO - PID 8584 | [8584, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2872 [task 2017-10-07T10:39:26.229Z] 10:39:26 INFO - (xpcshell/head.js) | test MAIN run_test pending (1) [task 2017-10-07T10:39:26.230Z] 10:39:26 INFO - (xpcshell/head.js) | test run_next_test 0 pending (2) [task 2017-10-07T10:39:26.230Z] 10:39:26 INFO - (xpcshell/head.js) | test MAIN run_test finished (2) [task 2017-10-07T10:39:26.230Z] 10:39:26 INFO - running event loop [task 2017-10-07T10:39:26.232Z] 10:39:26 INFO - PID 8584 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties [task 2017-10-07T10:39:26.234Z] 10:39:26 INFO - PID 8584 | [8584, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /builds/worker/workspace/build/src/dom/media/CubebUtils.cpp, line 370 [task 2017-10-07T10:39:26.236Z] 10:39:26 INFO - toolkit/components/places/tests/unifiedcomplete/test_autocomplete_on_value_removed_479089.js | Starting ensure_search_engine [task 2017-10-07T10:39:26.236Z] 10:39:26 INFO - (xpcshell/head.js) | test ensure_search_engine pending (2) [task 2017-10-07T10:39:26.242Z] 10:39:26 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2) [task 2017-10-07T10:39:26.247Z] 10:39:26 INFO - PID 8584 | [8584, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004002: file /builds/worker/workspace/build/src/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 182 [task 2017-10-07T10:39:26.251Z] 10:39:26 INFO - "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties" [task 2017-10-07T10:39:26.253Z] 10:39:26 INFO - PID 8584 | [8584, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /builds/worker/workspace/build/src/netwerk/protocol/res/SubstitutingProtocolHandler.cpp, line 253 [task 2017-10-07T10:39:26.255Z] 10:39:26 INFO - PID 8584 | JavaScript strict warning: jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/nsSearchService.js, line 762: ReferenceError: reference to undefined property "name" [task 2017-10-07T10:39:26.257Z] 10:39:26 INFO - (xpcshell/head.js) | test run_next_test 1 pending (2) [task 2017-10-07T10:39:26.259Z] 10:39:26 INFO - (xpcshell/head.js) | test ensure_search_engine finished (2) [task 2017-10-07T10:39:26.262Z] 10:39:26 INFO - "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "name"" {file: "jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/nsSearchService.js" line: 762}]" [task 2017-10-07T10:39:26.264Z] 10:39:26 INFO - toolkit/components/places/tests/unifiedcomplete/test_autocomplete_on_value_removed_479089.js | Starting test_autocomplete_on_value_removed [task 2017-10-07T10:39:26.270Z] 10:39:26 INFO - (xpcshell/head.js) | test test_autocomplete_on_value_removed pending (2) [task 2017-10-07T10:39:26.272Z] 10:39:26 INFO - PID 8584 | JavaScript error: /builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/places/tests/unifiedcomplete/test_autocomplete_on_value_removed_479089.js, line 28: uncaught exception: 2147500034 [task 2017-10-07T10:39:26.274Z] 10:39:26 INFO - PID 8584 | [8584, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, NS_ERROR_XPC_GS_RETURNED_FAILURE) failed with result 0x80004002: file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSID.cpp, line 702 [task 2017-10-07T10:39:26.276Z] 10:39:26 INFO - (xpcshell/head.js) | test run_next_test 1 finished (2) [task 2017-10-07T10:39:26.278Z] 10:39:26 INFO - Unexpected exception NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService] [task 2017-10-07T10:39:26.280Z] 10:39:26 INFO - test_autocomplete_on_value_removed@/builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/places/tests/unifiedcomplete/test_autocomplete_on_value_removed_479089.js:28:18 [task 2017-10-07T10:39:26.282Z] 10:39:26 INFO - async*asyncFunction@resource://gre/modules/Task.jsm:241:18 [task 2017-10-07T10:39:26.286Z] 10:39:26 INFO - Task_spawn@resource://gre/modules/Task.jsm:166:12 [task 2017-10-07T10:39:26.287Z] 10:39:26 INFO - _run_next_test@/builds/worker/workspace/build/tests/xpcshell/head.js:1488:9 [task 2017-10-07T10:39:26.289Z] 10:39:26 INFO - run@/builds/worker/workspace/build/tests/xpcshell/head.js:701:9 [task 2017-10-07T10:39:26.292Z] 10:39:26 INFO - _do_main@/builds/worker/workspace/build/tests/xpcshell/head.js:221:3 [task 2017-10-07T10:39:26.294Z] 10:39:26 INFO - _execute_test@/builds/worker/workspace/build/tests/xpcshell/head.js:544:5 [task 2017-10-07T10:39:26.297Z] 10:39:26 INFO - @-e:1:1 [task 2017-10-07T10:39:26.299Z] 10:39:26 INFO - exiting test [task 2017-10-07T10:39:26.301Z] 10:39:26 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "uncaught exception: 2147500034" {file: "/builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/places/tests/unifiedcomplete/test_autocomplete_on_value_removed_479089.js" line: 28}]"
Flags: needinfo?(mak77)
ah thanks, that test is a dumb dupe and I should remove it.
Flags: needinfo?(mak77)
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/autoland/rev/25cb665dd7bd deleting history from urlbar completion list doesn't work. r=adw
https://hg.mozilla.org/mozilla-central/rev/25cb665dd7bd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8915723 [details] Bug 1402555 - deleting history from urlbar completion list doesn't work. Approval Request Comment [Feature/Bug causing the regression]: bug 1391293 [User impact if declined]: deleting history entries from the location bar popup doesn't work [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: not yet [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?]: it's a simple and contained code change [String changes made/needed]: none
Attachment #8915723 - Flags: approval-mozilla-beta?
It works for me now, thanks!
Status: RESOLVED → VERIFIED
Aryx pinged me about some intermittent failures starting after this landed. The fix itself shouldn't cause those failures though. I have an half idea on what could be involved, in the test I forgot to close the AC popup, and this could maybe skew the timings for the next tests. I must note that those tests are already intermittent-prone, as I noted at https://bugzilla.mozilla.org/show_bug.cgi?id=1406860#c1. I'll land a follow-up patch closing the popup in this test, and try to find a more general solution to the problem in bug 1406860. ni? Aryx just as an heads-up.
Flags: needinfo?(aryx.bugmail)
Pushed by mak77@bonardo.net: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb1c5e40c33a followup to close the urlbar popup, since some tests are unhappy with the status quo. r=post-facto
Comment on attachment 8915723 [details] Bug 1402555 - deleting history from urlbar completion list doesn't work. Taking this fix assuming the intermittents are not due to a code problem but rather a fix needed in the tests (as noted by Mak), Beta57+
Attachment #8915723 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I've actually seen one of the failures, so I think it just reduced the likely for the intermittent. I will make the tests more reliable in bug 1406860.
(In reply to Marco Bonardo [::mak] from comment #18) > [Is this code covered by automated tests?]: yes > [Has the fix been verified in Nightly?]: not yet > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on Marco's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: