Closed
Bug 913713
Opened 11 years ago
Closed 11 years ago
Recording keyword searches for FHR fails with org.json.JSONException: No value for identifier
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox23 wontfix, firefox24 wontfix, firefox25 fixed, firefox26 fixed)
RESOLVED
FIXED
Firefox 26
People
(Reporter: Margaret, Assigned: mcomella)
References
Details
(Whiteboard: [qa+])
Attachments
(1 file, 4 obsolete files)
1.30 KB,
patch
|
rnewman
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
I found this while writing a patch for bug 909455, but it appears to be an issue that's existed all along. E/GeckoHealthRec(17235): Exception handling message "Search:Event": E/GeckoHealthRec(17235): org.json.JSONException: No value for identifier E/GeckoHealthRec(17235): at org.json.JSONObject.get(JSONObject.java:354) E/GeckoHealthRec(17235): at org.json.JSONObject.getString(JSONObject.java:514) E/GeckoHealthRec(17235): at org.mozilla.gecko.health.BrowserHealthRecorder.handleMessage(BrowserHealthRecorder.java:692) E/GeckoHealthRec(17235): at org.mozilla.gecko.util.EventDispatcher.dispatchEvent(EventDispatcher.java:96) E/GeckoHealthRec(17235): at org.mozilla.gecko.BrowserApp.recordSearch(BrowserApp.java:1500) E/GeckoHealthRec(17235): at org.mozilla.gecko.BrowserApp.access$1400(BrowserApp.java:84) E/GeckoHealthRec(17235): at org.mozilla.gecko.BrowserApp$28.run(BrowserApp.java:1475) E/GeckoHealthRec(17235): at android.os.Handler.handleCallback(Handler.java:730) E/GeckoHealthRec(17235): at android.os.Handler.dispatchMessage(Handler.java:92) E/GeckoHealthRec(17235): at android.os.Looper.loop(Looper.java:137) E/GeckoHealthRec(17235): at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:32)
Reporter | ||
Comment 1•11 years ago
|
||
Comment here says the identifier can be null: https://hg.mozilla.org/mozilla-central/annotate/a3cc1c802366/mobile/android/base/AwesomeBar.java#l354 Exception occurs in EVENT_SEARCH handler: https://hg.mozilla.org/mozilla-central/annotate/a3cc1c802366/mobile/android/base/health/BrowserHealthRecorder.java#l692 So it looks like there's a problem putting a null value in the JSON object.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → michael.l.comella
Assignee | ||
Comment 2•11 years ago
|
||
What are the differences between BrowserHealthRecorder.SEARCH_LOCATIONS [1]? My best guess: bartext - Clicking a search provider that has the same text as you typed in barsuggest - Clicking a search provider with suggestions rather than exact text (i.e. Google search suggestions) barkeyword - A keyword search (bookmark, add keyword, etc.) It doesn't seem like we're being consistent in the code. [1]: https://hg.mozilla.org/mozilla-central/file/bdbdeda69f05/mobile/android/base/health/BrowserHealthRecorder.java#l707
Flags: needinfo?(rnewman)
Comment 3•11 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #2) > What are the differences between BrowserHealthRecorder.SEARCH_LOCATIONS [1]? Between that and what? > My best guess: > bartext - Clicking a search provider that has the same text as you typed > in Hitting Enter/Go in the Awesomebar. > barsuggest - Clicking a search provider with suggestions rather than > exact text (i.e. Google search suggestions) Picking any suggestion. > barkeyword - A keyword search (bookmark, add keyword, etc.) Yup.
Flags: needinfo?(rnewman)
Comment 4•11 years ago
|
||
(In reply to :Margaret Leibovic from comment #1) > Comment here says the identifier can be null: > https://hg.mozilla.org/mozilla-central/annotate/a3cc1c802366/mobile/android/ > base/AwesomeBar.java#l354 Can be, but in most cases should not be -- it'll be "google", for example. > Exception occurs in EVENT_SEARCH handler: > https://hg.mozilla.org/mozilla-central/annotate/a3cc1c802366/mobile/android/ > base/health/BrowserHealthRecorder.java#l692 > > So it looks like there's a problem putting a null value in the JSON object. Fix might be as simple as using .optString instead of .getString.
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•11 years ago
|
||
> Between that and what?
I was unclear but you answered it - I meant each of the values in SEARCH_LOCATIONS.
Assignee | ||
Comment 7•11 years ago
|
||
Hey, Greg - I noticed that the "keyword-search" event does not actually notify for keyword searches, but rather for searches via the default search engine [1]. As such, I renamed the event to "default-search". You may notice that I left some Java components named inconsistently after this patch (e.g. EVENT_KEYWORD_SEARCH) - this is left for part 3 so rnewman can review those components separately. Let me know if you need more information about this. [1]: https://hg.mozilla.org/mozilla-central/file/4c431a919737/docshell/base/nsDefaultURIFixup.cpp#l361
Attachment #801946 -
Flags: review?(gps)
Assignee | ||
Comment 8•11 years ago
|
||
try: https://tbpl.mozilla.org/?tree=Try&rev=a107715a1c72
Assignee | ||
Comment 9•11 years ago
|
||
See part 2 for relevant context.
Attachment #801949 -
Flags: review?(rnewman)
Updated•11 years ago
|
Attachment #801932 -
Flags: review?(rnewman) → review+
Comment 11•11 years ago
|
||
Comment on attachment 801946 [details] [diff] [review] 02: Rename keyword-search to default-search Review of attachment 801946 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/nsBrowserGlue.js @@ -299,5 @@ > case "profile-before-change": > this._onProfileShutdown(); > break; > #ifdef MOZ_SERVICES_HEALTHREPORT > - case "keyword-search": This has me doubting our analysis... or perhaps desktop is incorrectly recording searches now? Did you check?
Comment 12•11 years ago
|
||
Comment on attachment 801950 [details] [diff] [review] 04: Update comment Review of attachment 801950 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/BrowserApp.java @@ +1458,5 @@ > } > > final String keywordUrl = BrowserDB.getUrlForKeyword(getContentResolver(), keyword); > > + // If there isn't a bookmark keyword, use loadUrl to fallback to the default search engine. Note that it might not actually be a search; it just means we're falling through.
Attachment #801950 -
Flags: review?(rnewman) → review+
Comment 13•11 years ago
|
||
Comment on attachment 801946 [details] [diff] [review] 02: Rename keyword-search to default-search This is more a browser patch and possibly has implications I'm not aware of. Gavin was all over the recent search engine refactor and should be aware of the underlying mechanisms, so reassigning review to him.
Attachment #801946 -
Flags: review?(gps) → review?(gavin.sharp)
Assignee | ||
Comment 14•11 years ago
|
||
> This has me doubting our analysis... or perhaps desktop is incorrectly > recording searches now? Did you check? By checking against about:healthreport on Nightly, the behavior seems correct. A default engine search increments org.mozilla.searches.counts.google.urlbar while a keyword search does not. Perhaps the confusion is this: [1] only executes if the default search engine is retrievable because it is creating a default search URI (from what I can understand), but then it issues a "keyword-search" event (rather than "default-search", which it probably should be doing and what the patch in part 2 does). [1]: https://hg.mozilla.org/mozilla-central/file/4c431a919737/docshell/base/nsDefaultURIFixup.cpp#l366
Assignee | ||
Comment 15•11 years ago
|
||
> Note that it might not actually be a search; it just means we're falling through. Are you sure? For this method, I believe we eliminate all the other cases: * If the user did not enter anything (the only case where we would not do anything) [1] * If the user entered a non-search query (we load it as a URL) [2] Thus, the remaining cases result in the user input being loaded as a query. We're handling the not a bookmark keyword case, which is a default search (since this is via editing mode) [3], with the other case being a bookmark keyword search. [1]: https://hg.mozilla.org/mozilla-central/file/4d152d0220be/mobile/android/base/BrowserApp.java#l1433 [2]: https://hg.mozilla.org/mozilla-central/file/4d152d0220be/mobile/android/base/BrowserApp.java#l1438 [3]: https://hg.mozilla.org/mozilla-central/file/4d152d0220be/mobile/android/base/BrowserApp.java#l1462
Comment 16•11 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #15) > Are you sure? isSearchQuery is a heuristic.
Assignee | ||
Comment 17•11 years ago
|
||
I made the comment more accurate.
Attachment #801950 -
Attachment is obsolete: true
Attachment #802436 -
Flags: review?(rnewman)
Updated•11 years ago
|
Attachment #802436 -
Flags: review?(rnewman) → review+
Comment 18•11 years ago
|
||
Holding off on reviewing Part 3 until Gavin either reviews part 2 or hands it off.
Assignee | ||
Comment 19•11 years ago
|
||
Part 1 actually fixes the issue described by this bug so I'm moving parts 2-4 to bug 915355 so the fix can be landed and uplifted.
Assignee | ||
Updated•11 years ago
|
Attachment #801946 -
Attachment is obsolete: true
Attachment #801946 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•11 years ago
|
Attachment #801949 -
Attachment is obsolete: true
Attachment #801949 -
Flags: review?(rnewman)
Assignee | ||
Updated•11 years ago
|
Attachment #802436 -
Attachment is obsolete: true
Assignee | ||
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4fb8c9f55c46
Assignee | ||
Comment 21•11 years ago
|
||
QA test plan: 1) Create a keyword bookmark (go to a page with search via a GET request - I used Yahoo, query for some content, create bookmark, edit the bookmark options, add a keyword, and replace the previously-specified query content with "%s") 2) Click the url bar 3) Type the previously entered keyword and some new query Ensure: * The keyword bookmark searches for the correct content * The logcat output does not contain a JSONException * The logcat output contains "Recording search: <likely-null>, barkeyword" It may also be worth verifying that other searches (i.e. clicking a suggestion, using the default search engine via the url bar) also work properly. They should each have their own "Recording search..." output ("barsuggestion" and "bartext" respectively).
Whiteboard: [qa+]
Assignee | ||
Comment 22•11 years ago
|
||
Comment on attachment 801932 [details] [diff] [review] 01: Use optString (fix the bug) [Approval Request Comment] Bug caused by (feature/regressing bug #): Unknown User impact if declined: Searching via keywords (e.g. in the url bar, "yahoo some query") will not work and will not alert the user why Testing completed (on m-c, etc.): Tested locally Risk to taking this patch (and alternatives if risky): Low risk - In the (highly unlikely) worst case, this code is touched by all search paths so I suppose all searches could be broken String or IDL/UUID changes made by this patch: None
Attachment #801932 -
Flags: approval-mozilla-aurora?
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4fb8c9f55c46
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment 24•11 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #21) > QA test plan: > 1) Create a keyword bookmark (go to a page with search via a GET request > - I used Yahoo, query for some content, create bookmark, edit the bookmark > options, add a keyword, and replace the previously-specified query content > with "%s") If you have Sync set up, you can right-click on an input field and choose "Add a Keyword for this search…", and let Sync take care of moving it to your phone.
Comment 25•11 years ago
|
||
Comment on attachment 801932 [details] [diff] [review] 01: Use optString (fix the bug) Good to skip up the trains to Aurora 25, but since this never worked for FHR, not tracking for other releases.
Attachment #801932 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
tracking-fennec: ? → ---
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•