Closed Bug 1057166 Opened 5 years ago Closed 5 years ago

Move FHR reporting of keyword searches out of nsDefaultURIFixup.getFixupURIInfo()

Categories

(Firefox :: Address Bar, defect)

30 Branch
defect
Not set
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.3

People

(Reporter: Unfocused, Assigned: Gijs)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 4 obsolete files)

Since bug 693808 introduced nsDefaultURIFixup.getFixupURIInfo(), we've been using what used to be createFixupURI() for things other than navigation (see also bug 951624). Unfortunately, there's a FHR probe in there for keyword searches - and the assumptions behind the placement of that are now invalid.

That probe should be moved out into consumers of the getFixupURIInfo() API, where we know that a keyword search is actually being used.
Flags: qe-verify-
Flags: firefox-backlog+
Sounds like we need this in Firefox 33 to avoid throwing off our search data, given that bug 693808 landed there.
Flags: qe-verify- → qe-verify+
(In reply to Blair McBride [:Unfocused] from comment #0)
> Since bug 693808 introduced nsDefaultURIFixup.getFixupURIInfo(), we've been
> using what used to be createFixupURI() for things other than navigation

We've been doing that all the time, look for createFixupURI in our tree:

http://mxr.mozilla.org/mozilla-central/search?string=createfixupuri

We didn't add cases in bug 693808, I don't think, so I'm not sure if this should really be prioritized specifically for 33. There also don't seem to be extra callers for getfixupuriinfo:

http://mxr.mozilla.org/mozilla-central/search?string=getfixupuriinfo

That doesn't change the fact that ideally, we should pass the search engine used into the fixupuriinfo object, and then use that in the callers for the fhr calls, but that can go together with the other changes and land for 34/35, IMO, as we've been shipping this version of the code for a fairly long time now.
It's possible that bug 982428 added the possibility of double-recording location bar searches in some edge cases. From a quick audit of 32 vs. 33, I don't see any other added cases of possible over-reporting. But, I do see a lot of possible existing cases of over-reporting (that have existed since we added FHR tracking of location bar searches).

So we probably don't need to track for 33, but we should really fix this stuff.
Points: --- → 8
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 35.1
Well, that was fun... Basically, I changed KeywordToURI to use an nsIURIFixup, and then had to mess with the e10s version of this as well. I don't know that we have any tests for that, but the other ones all pass with these changes (including the browser mochitest that checks for this notification, although I am not sure how thorough that is). I made docshell responsible for actually sending the notification. Try push:

remote:   https://tbpl.mozilla.org/?tree=Try&rev=83e54c80e581
remote: Alternatively, view them on Treeherder (experimental):
remote:   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=83e54c80e581
Attachment #8486537 - Flags: review?(bzbarsky)
Comment on attachment 8486537 [details] [diff] [review]
move keyword reporting out of defaulturifixup,

Hrm. I suspect that this change breaks the reporting on e10s, because the docshell that's calling in here is in content, and the previous situation led to nsDefaultURIFixup was being called in chrome... bz, can you check if my reasoning is sane, and/or how best to fix this? I guess we can forward the observer notification to the parent process again from the docshell? Kind of sucks, but I'm not sure what else there is to do...
Attachment #8486537 - Flags: review?(bzbarsky) → feedback?(bzbarsky)
> and the previous situation led to nsDefaultURIFixup was being called in chrome...

Who was the relevant nsDefaultURIFixup caller before?
(In reply to Boris Zbarsky [:bz] from comment #6)
> > and the previous situation led to nsDefaultURIFixup was being called in chrome...
> 
> Who was the relevant nsDefaultURIFixup caller before?

AFAICT, content processes currently call KeywordToURI (presumably from docshell), and hit this block:

http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDefaultURIFixup.cpp?mark=446-469#429

which calls SendKeywordToURI, which then means the chrome process calls into KeywordToURI again here:

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp?mark=3862-3863#3851

which would then send the notification.
I see.  So shouldn't the receiver of the SendKeywordToURI call just report?
This works. I fixed the test so it actually runs with the --e10s switch passed, and it passes locally.
Attachment #8487900 - Flags: review?(bzbarsky)
Attachment #8486537 - Attachment is obsolete: true
Attachment #8486537 - Flags: feedback?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #8)
> I see.  So shouldn't the receiver of the SendKeywordToURI call just report?

No, because not all of those calls will lead to a load.
Comment on attachment 8487900 [details] [diff] [review]
move FHR reporting out of docshell and fix test to work in e10s,

Mike, do you have time/interest to give this a first pass?
Attachment #8487900 - Flags: feedback?(mconley)
I do indeed.
Comment on attachment 8487900 [details] [diff] [review]
move FHR reporting out of docshell and fix test to work in e10s,

Review of attachment 8487900 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Gijs.

I've noticed that none of this nsIURIFixupInfo stuff is in the MDN page about nsIURIFixup: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIURIFixup. We should set dev-doc-needed here.

Just a few ideas below. I'm certainly open to argument on any of it, especially since up until now, I wasn't really n'sync with everything going on with keyword searches.

::: docshell/base/nsDefaultURIFixup.cpp
@@ +514,5 @@
>  }
>  
> +// Helper to deal with passing around uri fixup stuff
> +nsresult
> +nsDefaultURIFixup::KeywordToURIInternal(const nsACString & aURIString,

KeywordToURI takes the parameters in order of aURIString, aPostData, aFixupInfo... is there a good reason not to mirror this for consistency?

@@ +1037,5 @@
>      mFixupCreatedAlternateURI(false)
>  {
>    mOriginalInput = aOriginalInput;
> +  nsAutoString keywordProviderName;
> +  keywordProviderName.AssignLiteral("");

According to the IDL, this should be nullptr if no keyword search was done... shouldn't this string default to null then?

::: docshell/base/nsDocShell.cpp
@@ +4650,5 @@
>      loadInfo->SetReferrer(aReferringURI);
>      loadInfo->SetHeadersStream(aHeaderStream);
>      loadInfo->SetBaseURI(aBaseURI);
>  
> +    if (fixupInfo) {

Wait - so if we have fix-up info... that doesn't guarantee that we're doing a search, does it? Couldn't GetKeywordProviderName still return nullptr here? If so, why are we sending the notification? I see that you return early in NotifyKeywordSearchLoading... but perhaps we can check for that before we ever call NotifyKeywordSearchLoading instead?

@@ +13447,5 @@
> +  if (aProvider.IsEmpty()) {
> +    return;
> +  }
> +
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {

Another option here, and I'm just spit-balling here, would be to fire a chrome-only event with this information, and have content.js listen for that event, and send a message up to something in browser.js which does the job of firing the notification. That'd work the same for both the e10s and non-e10s case.

We do something similar for the MozEnteredDomFullscreen event.

It means less duplication, but a bit more complexity - but it also moves all of the MOZ_TOOLKIT_SEARCH stuff out of DocShell and into browser.js, where it makes more sense. It also means you don't have to add more IPDL stuff - we'll use the message manager over in Javascript land instead.

I'm not 100% sold on the idea, but just putting it out there.

::: docshell/base/nsIURIFixup.idl
@@ -40,2 @@
>     */
> -  readonly attribute boolean fixupUsedKeyword;

I'm still seeing this attribute being read by browser.js:

http://hg.mozilla.org/mozilla-central/file/bc7deafdac4b/browser/base/content/browser.js#l682

That'll need to be switched over.

::: dom/ipc/ContentParent.h
@@ +630,4 @@
>                                    OptionalURIParams* aURI) MOZ_OVERRIDE;
>  
> +    virtual bool RecvNotifyKeywordSearchLoading(const nsString &aProvider,
> +                                                const nsString &aKeyword) MOZ_OVERRIDE; 

Trailing whitespace.
Attachment #8487900 - Flags: feedback?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> Comment on attachment 8487900 [details] [diff] [review]
> move FHR reporting out of docshell and fix test to work in e10s,
> 
> Review of attachment 8487900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Gijs.
> 
> I've noticed that none of this nsIURIFixupInfo stuff is in the MDN page
> about nsIURIFixup:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIURIFixup. We should set dev-doc-needed here.
> 
> Just a few ideas below. I'm certainly open to argument on any of it,
> especially since up until now, I wasn't really n'sync with everything going
> on with keyword searches.
> 
> ::: docshell/base/nsDefaultURIFixup.cpp
> @@ +514,5 @@
> >  }
> >  
> > +// Helper to deal with passing around uri fixup stuff
> > +nsresult
> > +nsDefaultURIFixup::KeywordToURIInternal(const nsACString & aURIString,
> 
> KeywordToURI takes the parameters in order of aURIString, aPostData,
> aFixupInfo... is there a good reason not to mirror this for consistency?

Hmm, well, aFixupInfo is an inout-style param (callers should pass in non-null, as it gets modified) and aPostData is strictly an out param (pointer to pointer, which might be a non-nullptr when the function returns depending on whether the search engine uses POST data). In KeywordToURI, aFixupInfo is strictly an out param. Hence the difference. We could change KeywordToURI to use the same ordering, I suppose? OTOH, because the arguments are kind of different, I would prefer to keep some kind of distinction. Maybe I should pick a different method name - something like "UpdateFixupInfoWithKeyword" or similar?

> @@ +1037,5 @@
> >      mFixupCreatedAlternateURI(false)
> >  {
> >    mOriginalInput = aOriginalInput;
> > +  nsAutoString keywordProviderName;
> > +  keywordProviderName.AssignLiteral("");
> 
> According to the IDL, this should be nullptr if no keyword search was
> done... shouldn't this string default to null then?

I forgot to update the IDL comment. I would prefer empty string rather than null because it simplifies the C++ code, as far as I can tell - having to check for both null and empty string is harder, whereas JS doesn't care. Would you be OK with that?

> ::: docshell/base/nsDocShell.cpp
> @@ +4650,5 @@
> >      loadInfo->SetReferrer(aReferringURI);
> >      loadInfo->SetHeadersStream(aHeaderStream);
> >      loadInfo->SetBaseURI(aBaseURI);
> >  
> > +    if (fixupInfo) {
> 
> Wait - so if we have fix-up info... that doesn't guarantee that we're doing
> a search, does it? Couldn't GetKeywordProviderName still return nullptr
> here? If so, why are we sending the notification? I see that you return
> early in NotifyKeywordSearchLoading... but perhaps we can check for that
> before we ever call NotifyKeywordSearchLoading instead?

It seemed to me like it was better to check in NotifyKeywordSearchLoading than to check at the callsites, and hope that everyone who calls it also checks. I could add comments at the callsites...

> @@ +13447,5 @@
> > +  if (aProvider.IsEmpty()) {
> > +    return;
> > +  }
> > +
> > +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> 
> Another option here, and I'm just spit-balling here, would be to fire a
> chrome-only event with this information, and have content.js listen for that
> event, and send a message up to something in browser.js which does the job
> of firing the notification. That'd work the same for both the e10s and
> non-e10s case.

I'm not sure I understand - is content.js actually run in e10s? I didn't think it was. And what is a "chrome only event" ?

> We do something similar for the MozEnteredDomFullscreen event.
> 
> It means less duplication, but a bit more complexity - but it also moves all
> of the MOZ_TOOLKIT_SEARCH stuff out of DocShell and into browser.js, where
> it makes more sense. It also means you don't have to add more IPDL stuff -
> we'll use the message manager over in Javascript land instead.
> 
> I'm not 100% sold on the idea, but just putting it out there.

Hmm. Sounds compelling on its face, although it seems like this would only work in cases where there's a content.js associated with the docshell. That's probably the case for 99.99% of the docshells we care about (ie browser tabs), but I'm not sure it's the case for 100%, and I'd like not to go from over-reporting to under-reporting in this bug...

> ::: docshell/base/nsIURIFixup.idl
> @@ -40,2 @@
> >     */
> > -  readonly attribute boolean fixupUsedKeyword;
> 
> I'm still seeing this attribute being read by browser.js:
> 
> http://hg.mozilla.org/mozilla-central/file/bc7deafdac4b/browser/base/content/
> browser.js#l682
> 
> That'll need to be switched over.

Good point.
Keywords: dev-doc-needed
Re my own:
> We could change KeywordToURI to use the same ordering, I suppose? 

not really, because the nsIURIFixupInfo is the idl return value, and we really don't want to make the postdata the return value. :-\
When this patch is almost ready, would you mind double-checking that urlbar keyword searches still show up in about:telemetry's Simple Measurements >> UITelemetry section under the "urlbar-keyword" key when the patch is applied?  (I suspect they will, but I'm very interested in not regressing bug 1038248.  ;)  Thanks!
Sometimes when you reread what you wrote the day after you seem incredibly stupid:

(In reply to :Gijs Kruitbosch from comment #14)
> I'm not sure I understand - is content.js actually run in e10s?

I meant run in *non-e10s*.
(In reply to :Gijs Kruitbosch from comment #14)
> Hmm, well, aFixupInfo is an inout-style param (callers should pass in
> non-null, as it gets modified) and aPostData is strictly an out param
> (pointer to pointer, which might be a non-nullptr when the function returns
> depending on whether the search engine uses POST data). In KeywordToURI,
> aFixupInfo is strictly an out param. Hence the difference. We could change
> KeywordToURI to use the same ordering, I suppose? OTOH, because the
> arguments are kind of different, I would prefer to keep some kind of
> distinction. Maybe I should pick a different method name - something like
> "UpdateFixupInfoWithKeyword" or similar?
> 

I think renaming the method is fine.

> > @@ +1037,5 @@
> > >      mFixupCreatedAlternateURI(false)
> > >  {
> > >    mOriginalInput = aOriginalInput;
> > > +  nsAutoString keywordProviderName;
> > > +  keywordProviderName.AssignLiteral("");
> > 
> > According to the IDL, this should be nullptr if no keyword search was
> > done... shouldn't this string default to null then?
> 
> I forgot to update the IDL comment. I would prefer empty string rather than
> null because it simplifies the C++ code, as far as I can tell - having to
> check for both null and empty string is harder, whereas JS doesn't care.
> Would you be OK with that?
> 

I think I'm fine with the empty string, so long as we update the IDL documentation, yeah.

> > ::: docshell/base/nsDocShell.cpp
> > @@ +4650,5 @@
> > >      loadInfo->SetReferrer(aReferringURI);
> > >      loadInfo->SetHeadersStream(aHeaderStream);
> > >      loadInfo->SetBaseURI(aBaseURI);
> > >  
> > > +    if (fixupInfo) {
> > 
> > Wait - so if we have fix-up info... that doesn't guarantee that we're doing
> > a search, does it? Couldn't GetKeywordProviderName still return nullptr
> > here? If so, why are we sending the notification? I see that you return
> > early in NotifyKeywordSearchLoading... but perhaps we can check for that
> > before we ever call NotifyKeywordSearchLoading instead?
> 
> It seemed to me like it was better to check in NotifyKeywordSearchLoading
> than to check at the callsites, and hope that everyone who calls it also
> checks. I could add comments at the callsites...
> 

It just seems a bit strange to call a method that clearly notifies something, but actually only *maybe* notifies something. Perhaps this should be called MaybeNotifyKeywordSearchLoading, if we're going to keep using it without distinction at the call sites. *shrug*.


> > @@ +13447,5 @@
> > > +  if (aProvider.IsEmpty()) {
> > > +    return;
> > > +  }
> > > +
> > > +  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> > 
> > Another option here, and I'm just spit-balling here, would be to fire a
> > chrome-only event with this information, and have content.js listen for that
> > event, and send a message up to something in browser.js which does the job
> > of firing the notification. That'd work the same for both the e10s and
> > non-e10s case.
> 
> I'm not sure I understand - is content.js actually run in e10s? I didn't
> think it was. And what is a "chrome only event" ?
> 
> > We do something similar for the MozEnteredDomFullscreen event.
> > 
> > It means less duplication, but a bit more complexity - but it also moves all
> > of the MOZ_TOOLKIT_SEARCH stuff out of DocShell and into browser.js, where
> > it makes more sense. It also means you don't have to add more IPDL stuff -
> > we'll use the message manager over in Javascript land instead.
> > 
> > I'm not 100% sold on the idea, but just putting it out there.
> 
> Hmm. Sounds compelling on its face, although it seems like this would only
> work in cases where there's a content.js associated with the docshell.
> That's probably the case for 99.99% of the docshells we care about (ie
> browser tabs), but I'm not sure it's the case for 100%, and I'd like not to
> go from over-reporting to under-reporting in this bug...
> 

I talked with Gijs about this over IRC, and while what I'm suggesting is possible, it's unclear whether it would at all simplify things (though it would reduce duplication). We'll defer to bz.
QA Contact: kamiljoz
This leaves the arch more-or-less as-is, but addresses all the other comments. I'll add another patch that uses the other approach for comparison, hopefully later today.
Attachment #8489308 - Flags: review?(bzbarsky)
Attachment #8487900 - Attachment is obsolete: true
Attachment #8487900 - Flags: review?(bzbarsky)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> Another option here, and I'm just spit-balling here, would be to fire a
> chrome-only event with this information, and have content.js listen for that
> event, and send a message up to something in browser.js which does the job
> of firing the notification. That'd work the same for both the e10s and
> non-e10s case.
> 
> We do something similar for the MozEnteredDomFullscreen event.

Except then I realized it doesn't seem like I can pass in arbitrary data for this event? Id est, no event.detail parameter? And I need to pass 2 strings (search engine name and the keyword... so that is kind of a bummer).

At this point, I think we should take the current patch (assuming it gets r+ and so on) and file a followup if we think we can improve the situation here further.
Iteration: 35.1 → 35.2
Iteration: 35.2 → 35.3
Boris, another gentle review ping? Would really like to land this in 35 still, if possible. :-)
Comment on attachment 8489308 [details] [diff] [review]
move FHR reporting out of docshell and fix test to work in e10s,

Sorry.  This fell through the cracks.  :(

>+        info->mKeywordAsSent = NS_ConvertUTF8toUTF16(keyword);

  CopyUTF8toUTF16(keyword, info->mKeywordAsSent);

>+            nsAutoString keywordW = NS_ConvertUTF8toUTF16(keyword);

  NS_ConvertUTF8toUTF16 keywordW(keyword);

>+  nsAutoString keywordProviderName;
>+  keywordProviderName.AssignLiteral("");
>+  mKeywordProviderName = keywordProviderName;

This seems like a complicated no-op, since mKeywordProviderName starts out as an empty string.

Also, you should probably make the two new string members nsString, not nsAutoString.

>+                    keywordProviderName.AssignLiteral("");
>+                    keywordAsSent.AssignLiteral("");

  keywordProviderName.Truncate();

and similar for the other variable.

r=me
Attachment #8489308 - Flags: review?(bzbarsky) → review+
That didn't last long... backed out because:

docshell/base/nsDefaultURIFixup.cpp:1027:14: error: variable 'rv' set but not used [-Werror=unused-but-set-variable]

I think some of this actually needs to be passed down (rather than not caring about rv) so I backed out.


https://hg.mozilla.org/integration/fx-team/rev/46db5df0e03f
Attachment #8498511 - Attachment is obsolete: true
Attachment #8498511 - Flags: review?(bzbarsky)
Attached patch InterdiffSplinter Review
Attachment #8489308 - Attachment is obsolete: true
Comment on attachment 8498513 [details] [diff] [review]
move FHR reporting out of docshell and fix test to work in e10s,

r=me based on interdiff (for which many thanks!)
Attachment #8498513 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/3b0e81a6e214
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
I've tested this in Firefox 35 Beta 6 (BuildID=20141222200458) on Windows 7 x64. I tried doing some keyword searches for Google and Amazon (in the URL bar) then checked about:healthreport Raw Data and about:telemetry Simple Measurements - > UI Telemetry, but did not find any entry for keyword searches. Am I missing something here?
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #31)
> I've tested this in Firefox 35 Beta 6 (BuildID=20141222200458) on Windows 7
> x64. I tried doing some keyword searches for Google and Amazon (in the URL
> bar) then checked about:healthreport Raw Data and about:telemetry Simple
> Measurements - > UI Telemetry, but did not find any entry for keyword
> searches. Am I missing something here?

In about:healthreport, Raw Data, search for "<searchprovider>.urlbar" (under org.mozilla.searches.counts) -- seems to work on my copy of 35 beta, at least. Can you confirm? :-)
Flags: needinfo?(florin.mezei)
Note that the naming is confusing here. This is for searches from a simple text string in the URL bar, such as "mozilla firefox", not from the keywords used for the search engines, so this doesn't count setting "g" as the keyword for google and then putting "g mozilla firefox" in the url bar.
(In reply to :Gijs Kruitbosch from comment #32)
> In about:healthreport, Raw Data, search for "<searchprovider>.urlbar" (under
> org.mozilla.searches.counts) -- seems to work on my copy of 35 beta, at
> least. Can you confirm? :-)

Gijs I do get this on 35, but I thought this was about a separate entry for keyword searches. Did I understand this the wrong way... is there no such entry that should show up?
Flags: needinfo?(florin.mezei)
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #34)
> (In reply to :Gijs Kruitbosch from comment #32)
> > In about:healthreport, Raw Data, search for "<searchprovider>.urlbar" (under
> > org.mozilla.searches.counts) -- seems to work on my copy of 35 beta, at
> > least. Can you confirm? :-)
> 
> Gijs I do get this on 35, but I thought this was about a separate entry for
> keyword searches. Did I understand this the wrong way... is there no such
> entry that should show up?

I don't think that we have a separate entry for the "g mozilla" or "y firefox" type things for keywords off search engines. mconnor/florian: am I wrong and/or should we have one? Seems like we should...
Flags: needinfo?(mconnor)
Flags: needinfo?(florian)
(certainly that type of search isn't the bit of code that got moved in this bug, and so we should be OK to mark this verified assuming the .urlbar search counts are working as expected)
(In reply to :Gijs Kruitbosch from comment #36)
> (certainly that type of search isn't the bit of code that got moved in this
> bug, and so we should be OK to mark this verified assuming the .urlbar
> search counts are working as expected)

Thanks Gijs for clarifying things! I checked that the ".urlbar" entry is correctly counted when searching from the bar (excluding keyword searches) for all search engines, on Windows 7 x64, Mac OS X 10.9.5, Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
(In reply to Gijs Kruitbosch (Gone until Jan 5) from comment #35)
> I don't think that we have a separate entry for the "g mozilla" or "y
> firefox" type things for keywords off search engines. mconnor/florian: am I
> wrong and/or should we have one? Seems like we should...

Indeed we don't:
https://hg.mozilla.org/mozilla-central/annotate/57e4e9c33bef/browser/base/content/browser.js#l2083

I filed bug 1117153.
Flags: needinfo?(mconnor)
Flags: needinfo?(florian)
You need to log in before you can comment on or make changes to this bug.