Closed Bug 913713 Opened 6 years ago Closed 6 years ago

Recording keyword searches for FHR fails with org.json.JSONException: No value for identifier

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 26
Tracking Status
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: Margaret, Assigned: mcomella)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file, 4 obsolete files)

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)
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: nobody → michael.l.comella
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)
(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)
(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
> Between that and what?

I was unclear but you answered it - I meant each of the values in SEARCH_LOCATIONS.
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)
See part 2 for relevant context.
Attachment #801949 - Flags: review?(rnewman)
Attached patch 04: Update comment (obsolete) — Splinter Review
Part 4 of 4.
Attachment #801950 - Flags: review?(rnewman)
Attachment #801932 - Flags: review?(rnewman) → review+
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 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 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)
> 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
> 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
(In reply to Michael Comella (:mcomella) from comment #15)

> Are you sure?

isSearchQuery is a heuristic.
Attached patch 04a: Update comment (obsolete) — Splinter Review
I made the comment more accurate.
Attachment #801950 - Attachment is obsolete: true
Attachment #802436 - Flags: review?(rnewman)
Attachment #802436 - Flags: review?(rnewman) → review+
Holding off on reviewing Part 3 until Gavin either reviews part 2 or hands it off.
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.
Attachment #801946 - Attachment is obsolete: true
Attachment #801946 - Flags: review?(gavin.sharp)
Attachment #801949 - Attachment is obsolete: true
Attachment #801949 - Flags: review?(rnewman)
Attachment #802436 - Attachment is obsolete: true
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+]
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?
https://hg.mozilla.org/mozilla-central/rev/4fb8c9f55c46
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
(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 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+
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.