Closed Bug 1083469 Opened 10 years ago Closed 9 years ago

Breakdown: allow keywords APIs to work in parallel with Bookmarks.jsm

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
5

Tracking

()

RESOLVED FIXED
mozilla38
Iteration:
38.1 - 26 Jan

People

(Reporter: mak, Assigned: mak)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(2 obsolete files)

once we have sync and async APIs, the old keywords API might be broken due to the internal caching, we should fix that and provide new keyword APIs if they are missing.
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
I don't think there's any better way to do this than removing the cache (So both APIs access the same underlying data).

unfortunately that means consumers will hit the disk every time. The consumer I'm mostly worried about though, is autocomplete, through its usage of getURIForKeyword. We might need to introduce a cache in PlacesUtils that keeps a keyword->uri map updated based on notifications. Then autocomplete could use that cache.
also getShortcutOrURIAndPostData would benefit
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 36.3
So, the problem here is getShortcutOrURIAndPostData uses a synchronous callback. That was done on purpose to be able to convert Task.jsm to resolve-on-next-tick promises without having to fix too many tests that are relying on same-tick resolution.

Unfortunately we are again hitting the same issue, where getShortcutOrURIAndPostData should be async but it's not, and we'll hit it again in future.

Another interesting thing is that when we use getCharsetForURI, the callback is effectively async already... So currently that method is half-and-half.

I will see on Try how many tests are involved here, and decide based on that if it's worth to finally fix this properly, or delay again to another bug.
here is all of the tests failing due to async getShortcutOrURIAndPostData


INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_getshortcutoruri.js

INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_locationBarCommand.js

INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_urlbarEnter.js

INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_urlbarStop.js

INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug1064280_changeUrlInPinnedTab.js

INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/browser/browser_search_notification.js

INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/docshell/test/browser/browser_uriFixupIntegration.js
I think I will split the getShortcutOrURL part to a separate bug, since it is a less hot path than autocomplete that should be fine and soon or later we'll convert all consumers regardless.
Blocks: 1095424
Blocks: 1100294
Filed bug 1100291 and made it block bug 1100294
Summary: Fix keywords APIs → Allow keywords APIs to work in parallel with Bookmarks.jsm
Attached file MozReview Request: bz://1083469/mak (obsolete) —
Attachment #8523913 - Flags: review?(mano)
/r/719 - Bug 1083469 - Allow to use old keywords APIs along with Bookmarks.jsm r=mano

Pull down this commit:

hg pull review -r b4667a5cf865922d89d46b546661ce35b5be4f84
https://reviewboard.mozilla.org/r/717/#review437

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 1)
> +    let observer = {

Since KeywordsCache is an internal object, wouldn't it be better to make it implement the observer?

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 1)
> +      __noSuchMethod__() {}, // Catch all remaining onItem* methods.

BTW, now that we've Object.assign, I think I'm going to file a bug on intorducing PlacesUtils.ImplementBookmarksObserver({ just the functions you want })

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 1)
> +      yield this._reloadPromise

missing ;

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 1)
> +          this._reloadCache().catch(Cu.reportError);

Add a comment here starting that the cache should be readily avaiable as fast as possible. Otherwise someone may optimize this to just unset _initialized.

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 1)
> +          dump(`\nitemchanged reload\n\n`);

remove that dump.

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 1)
> +                                            , Ci.nsIAnnotationObserver ]),

Remove nsIAnnotationObserver.

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 1)
> +  _reloadCache: Task.async(function* () {

Just _reloadCache(), since you use Task.spawn.

::: toolkit/components/places/PlacesUtils.jsm
(Diff revision 1)
> +   * Gets the href and its associated post data for a given keyword, if any.

s/its associated/the/

::: toolkit/components/places/nsPlacesAutoComplete.js
(Diff revision 1)
> -        PlacesUtils.bookmarks.getURIForKeyword(this._currentSearchString)) {
> +          (yield PlacesUtils.bookmarks.getURIForKeyword(this._currentSearchString)).href) {

I guess you meant to use the new async method here.
Attachment #8523913 - Flags: review?(mano) → review+
Attachment #8523913 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/ce637f8721d3
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/ce637f8721d3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I backed out this because tests were failing when disabling unified complete

https://hg.mozilla.org/mozilla-central/rev/6467bdc11662

failures in
browser/base/content/test/general/browser_bug304198.js
browser/base/content/test/general/browser_bug559991.js
browser/base/content/test/general/browser_bug817947.js
browser/base/content/test/general/browser_canonizeURL.js

looks like old autocomplete doesn't love a Task.spawn...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Iteration: 36.3 → 37.1
Attached patch patch v2 (obsolete) — Splinter Review
I did a few changes:

nsPlacesAutocomplete is now yielding, that means a search can be canceled during the yield.
Thus I introduced a _pendingSearch to check if after the yield the query is still valid. note we cannot use _pendingQuery cause that's tracking the sqlite query instead. the new code in Unified is better organized...

UnifiedComplete is missing a couple pending checks, it should check pending after every yield.

finally, there was a race condition in the KeywordsCache initialization, since _initialized was set at the end, we were calling _initialized multiple times, and addObserver was throwing (new test added for this).

These 2 are all the changes I did, I hope this helps simplifying the review.

Tests are now passing locally, still I'm pushing to try.
Attachment #8523913 - Attachment is obsolete: true
Attachment #8530450 - Flags: review?(mano)
hm, e10s BC1 is unhappy yet
 chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug559991.js | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAutoCompleteController.getImageAt] at chrome://global/content/bindings/autocomplete.xml:1144
Blocks: 1095425
Comment on attachment 8530450 [details] [diff] [review]
patch v2

Cancelling review request both because the disturbing test failure and because Marco and I figured we need some sort of public access to the keywords cache, so that future Bookmarks.fetchTree/current PU.promiseBookmarksTree can make use of it. For this reason I also think it makes sense to have the cache object in Bookmarks.jsm rather than in PlacesUtils.jsm.
Attachment #8530450 - Flags: review?(mano)
Iteration: 37.1 → 37.2
Iteration: 37.2 → 37.3
Iteration: 37.3 - 12 Jan → 38.1 - 26 Jan
This is becoming more complex than it was, so I'm going to convert it to a breakdown and file new bugs for the single pieces.
Summary: Allow keywords APIs to work in parallel with Bookmarks.jsm → Breakdown: allow keywords APIs to work in parallel with Bookmarks.jsm
Depends on: 1125113
Depends on: 1125115
Depends on: 1125116
Depends on: 1125117
Depends on: 1125118
All the dependancies are filed, will add the filed bugs to the backlog generation sheet.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Attachment #8530450 - Attachment is obsolete: true
Target Milestone: mozilla36 → mozilla38
Depends on: 1249330
See Also: → 1326608
Depends on: 1326762
You need to log in before you can comment on or make changes to this bug.