flypig.co.uk

It's already the evening here while I start this post. I've not had a chance to do any gecko development during the day but am hoping to make up for some lost time now.

Yesterday we were looking at search in the address bar. We made a bit of progress: previously there were a couple of errors that simply stopped all search in its tracks. After having fixed them I'm not getting a page stating that "The address isn't valid". I think that's a step forwards!

There are still a couple of errors remaining, one of which we tracked down to some code in the URIFixup.jsm file. The task now is to figure out what changed between ESR 78 and ESR 91 that might make this error occur.

Taking a look through each of them side by side, the code in the getFixupURIInfo() method has changed quite a bit, but ultimately it arrives in the same place and there's no evidence in the ESR 78 code that the hand-off to search happens within this method. The return value is the same. So maybe it's worth taking a look at the code that calls this method instead.

Searching through the code for instances of getFixupURIInfo() is unsatisfactory (it's not clear which is the relevant one) so I'm going to try to tackle it from the other end. When you enter some text into the address bar and hit enter the following JavaScript is executed from inside the QML (this is from Overlay.qml in the sailfish-browser repository):

    function loadPage(url, newTab) {
        if (url == "about:config") {
            pageStack.animatorPush(Qt.resolvedUrl("ConfigWarning.qml"),
                {"browserPage": browserPage})
        } else if (url == "about:settings") {
            pageStack.animatorPush(Qt.resolvedUrl("../SettingsPage.qml"))
        } else {
            if (webView && webView.tabModel.count === 0) {
                webView.clearSurface();
            }
            // let gecko figure out how to handle malformed URLs
            var pageUrl = url
            if (!isNaN(pageUrl) && pageUrl.trim()) {
                pageUrl = "\"" + pageUrl.trim() + "\""
            }

            if (!searchField.enteringNewTabUrl && !newTab) {
                webView.releaseActiveTabOwnership()
                webView.load(pageUrl)
            } else {
                // Loading will start once overlay animator has animated chrome
                // visible.
                enteredUrl = pageUrl
            }
        }

        overlayAnimator.showChrome()
    }

The interesting parts here are the text "let gecko figure out how to handle malformed URLs" and the action, which is the webView.load(pageUrl) call. This is where we transition from sailfish-browser to qtmozembed.

So this is a call to QuickMozView::load() which calls the private method which does nothing other than call QMozViewPrivate::load() with the same parameter. This is where we transition from qtmozembed to EmbedLite in gecko-dev.

This is a call to mozilla::embedlite::EmbedLiteView::LoadURL() which we can find in embedding/embedlite/EmbedLiteView.cpp and which looks like this:

void
EmbedLiteView::LoadURL(const char* aUrl)
{
  LOGT("url:%s", aUrl);
  Unused << mViewParent->SendLoadURL(NS_ConvertUTF8toUTF16(
      nsDependentCString(aUrl)));
}

From the header file we can see that mViewParent is an instance of PEmbedLiteViewParent. This ends up as a call to EmbedLiteViewChild::RecvLoadURL(). This is an interesting method so worth copying out in full. Specifically interesting because of the flags it sets and passes on through a call to nsIWebNavigation::LoadURI():

mozilla::ipc::IPCResult EmbedLiteViewChild::RecvLoadURL(const nsString &url)
{
  LOGT("url:%s", NS_ConvertUTF16toUTF8(url).get());
  NS_ENSURE_TRUE(mWebNavigation, IPC_OK());

  uint32_t flags = 0;
  if (sAllowKeyWordURL) {
    flags |= nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
    flags |= nsIWebNavigation::LOAD_FLAGS_FIXUP_SCHEME_TYPOS;
  }
  flags |= nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL;

  LoadURIOptions loadURIOptions;
  loadURIOptions.mTriggeringPrincipal = nsContentUtils::GetSystemPrincipal();
  loadURIOptions.mLoadFlags = flags;
  mWebNavigation->LoadURI(url, loadURIOptions);

  return IPC_OK();
}

It's worth noting that nsIWebNavigation is just an interface. The mWebNavigation implementation of it comes from nsWebBrowser. All this does is pass the parameters along to nsDocShell::LoadURI(). We're deep in C++ territory now, but at any moment we're going to burst out into some JavaScript.

The code in nsDocShell::LoadURI() is considerably more complex. At the top of the method it contains this interesting sequence:

  RefPtr<nsDocShellLoadState> loadState;
  nsresult rv = nsDocShellLoadState::CreateFromLoadURIOptions(
      mBrowsingContext, aURI, aLoadURIOptions, getter_AddRefs(loadState));

  uint32_t loadFlags = aLoadURIOptions.mLoadFlags;
  if (NS_ERROR_MALFORMED_URI == rv) {
    MOZ_LOG(gSHLog, LogLevel::Debug,
            ("Creating an active entry on nsDocShell %p to %s (because "
             "we're showing an error page)",
             this, NS_ConvertUTF16toUTF8(aURI).get()));

This is interesting because of the MOZ_LOG output, which is currently hidden, but which we can enable.

These log messages can be controlled by category (in this case gSHLog which is set to "SessionHistory") and by log level (in this case Debug which takes the value 4). To view them, we set the MOZ_LOG environment variable, like this:

MOZ_LOG="SessionHistory:4" sailfish-browser

With this enabled we see the following output:

JavaScript error: resource://gre/modules/URIFixup.jsm, line 443:
    NS_ERROR_MALFORMED_URI: Couldn't build a valid uri
[Parent 6189: Unnamed thread 705c002670]: D/SessionHistory Creating an active
    entry on nsDocShell 705c999460 to test search (because we're showing an
    error page)

On the ESR 78 install I get neither of those errors. So, as suspected, the NS_ERROR_MALFORMED_URI return value is being caught and changed before it reaches the DocShell somewhere inside the nsDocShellLoadState::CreateFromLoadURIOptions() call.

And now we've reach the middle point from either end. Here's the bit of code inside this method running up to the call to GetFixupURIInfo() that's generating the error:

nsresult nsDocShellLoadState::CreateFromLoadURIOptions(
    BrowsingContext* aBrowsingContext, const nsAString& aURI,
    const LoadURIOptions& aLoadURIOptions, nsDocShellLoadState** aResult) {
[...]
  nsAutoString searchProvider, keyword;
  bool didFixup = false;
  if (fixup) {
    uint32_t fixupFlags =
        WebNavigationFlagsToFixupFlags(uri, uriString, loadFlags);

    // If we don't allow keyword lookups for this URL string, make sure to
    // update loadFlags to indicate this as well.
    if (!(fixupFlags & nsIURIFixup::FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP)) {
      loadFlags &= ~nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
    }
    // Ensure URIFixup will use the right search engine in Private Browsing.
    if (aBrowsingContext->UsePrivateBrowsing()) {
      fixupFlags |= nsIURIFixup::FIXUP_FLAG_PRIVATE_CONTEXT;
    }

    RefPtr<nsIInputStream> fixupStream;
    if (!XRE_IsContentProcess()) {
      nsCOMPtr<nsIURIFixupInfo> fixupInfo;
      sURIFixup->GetFixupURIInfo(uriString, fixupFlags,
                                 getter_AddRefs(fixupInfo));
[...]

Inside the GetFixupURIInfo() method there are a couple of conditions that look a like this:

    // If we still haven't been able to construct a valid URI, try to force a
    // keyword match.
    if (keywordEnabled && fixupFlags & FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP) {
      tryKeywordFixupForURIInfo(info.originalInput, info, isPrivateContext);
    }

Placing some debug output into GetFixupURIInfo() I can see that these aren't entered because of the flags: neither the FIXUP_FLAG_FIX_SCHEME_TYPOS nor the FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP flag are set.

The answer to why that is might be in the WebNavigationFlagsToFixupFlags() method that we can see above inside CreateFromLoadURIOptions(). Now while GetFixupURIInfo() is JavaScript, the class that's calling it is C++, so I can debug and step through that easily. I'm going to give that a go to see whether that sheds any further light on things.

Thread 8 "GeckoWorkerThre" hit Breakpoint 1, nsDocShellLoadState::
    CreateFromLoadURIOptions (aBrowsingContext=0x7f88cccb40, aURI=...,
    aLoadURIOptions=..., aResult=aResult@entry=0x7f9f3f23c8) at
    /usr/src/debug/xulrunner-qt5-91.9.1-1.aarch64/docshell/base/
    nsDocShellLoadState.cpp:226
226         const LoadURIOptions& aLoadURIOptions, nsDocShellLoadState** aResult) {
(gdb) p /x aLoadURIOptions.mLoadFlags
$5 = 0x40000
(gdb) n
227       uint32_t loadFlags = aLoadURIOptions.mLoadFlags;
(gdb) n
233       nsCOMPtr<nsIURI> uri;
(gdb) n
[LWP 25851 exited]
234       nsCOMPtr<nsIInputStream> postData(aLoadURIOptions.mPostData);
(gdb) n
237       NS_ConvertUTF16toUTF8 uriString(aURI);
(gdb) n
239       uriString.Trim(" ");
(gdb) n
241       uriString.StripCRLF();
(gdb) p uriString.mData
$6 = (mozilla::detail::nsTStringRepr<char>::char_type *) 0x7f9f3f2204 "abc def"
(gdb) n
245       rv = NS_NewURI(getter_AddRefs(uri), uriString);
(gdb) n
252       } else if (!sURIFixup && !XRE_IsContentProcess()) {
(gdb) n
262       nsAutoString searchProvider, keyword;
(gdb) p fixup
$7 = true
(gdb) n
264       if (fixup) {
(gdb) n

Thread 8 "GeckoWorkerThre" hit Breakpoint 2, WebNavigationFlagsToFixupFlags
    (aNavigationFlags=262144, aURIString=..., aURI=0x0)
    at /usr/src/debug/xulrunner-qt5-91.9.1-1.aarch64/docshell/base/
    nsDocShellLoadState.cpp:211
211       if (aURI) {
(gdb) p /x aNavigationFlags
$8 = 0x40000
(gdb) n
nsDocShellLoadState::CreateFromLoadURIOptions (aBrowsingContext=0x7f88cccb40,
    aURI=..., aLoadURIOptions=..., aResult=aResult@entry=0x7f9f3f23c8)
    at /usr/src/debug/xulrunner-qt5-91.9.1-1.aarch64/docshell/base/
    nsDocShellLoadState.cpp:266
266             WebNavigationFlagsToFixupFlags(uri, uriString, loadFlags);
(gdb) n
270         if (!(fixupFlags & nsIURIFixup::FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP)) {
(gdb) n
271           loadFlags &= ~nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
(gdb) n
274         if (aBrowsingContext->UsePrivateBrowsing()) {
(gdb) n
278         RefPtr<nsIInputStream> fixupStream;
(gdb) p /x fixupFlags
$9 = 0x0
(gdb) n
279         if (!XRE_IsContentProcess()) {
(gdb) n
280           nsCOMPtr<nsIURIFixupInfo> fixupInfo;
(gdb) n
281           sURIFixup->GetFixupURIInfo(uriString, fixupFlags,
(gdb) p fixupFlags
$10 = 0
(gdb) 

From this it looks like the problem has already happened by the time CreateFromLoadURIOptions() has been called with aLoadURIOptions.mLoadFlags set to 0x40000. The values we're interested in are the following, so we'd really want at least one of these taken from docshell/base/nsIWebNavigation.idl to be set:

  const unsigned long LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP = 0x100000;
  const unsigned long LOAD_FLAGS_FIXUP_SCHEME_TYPOS = 0x200000;

This takes us all the way back to EmbedLiteViewChild::RecvLoadURL() which I already copied out above. There we can see that these flags are dependent on the value of sAllowKeyWordURL. This is a global static variable, which is being set to false. Maybe that's our problem there.

Setting the flags using the debugger doesn't seem to make any difference:

Thread 8 "GeckoWorkerThre" hit Breakpoint 3, mozilla::embedlite::
    EmbedLiteViewChild::RecvLoadURL (this=0x7f88d03170, url=...)
    at /usr/src/debug/xulrunner-qt5-91.9.1-1.aarch64/mobile/sailfishos/
    embedshared/EmbedLiteViewChild.cpp:534
534     {
(gdb) p sAllowKeyWordURL
$11 = <optimized out>
(gdb) n
535       LOGT("url:%s", NS_ConvertUTF16toUTF8(url).get());
(gdb) n
545       LoadURIOptions loadURIOptions;
(gdb) p /x flags
$13 = 0x40000
(gdb) n
546       loadURIOptions.mTriggeringPrincipal = nsContentUtils::GetSystemPrincipal();
(gdb) n
547       loadURIOptions.mLoadFlags = flags;
(gdb) n
548       mWebNavigation->LoadURI(url, loadURIOptions);
(gdb) set loadURIOptions.mLoadFlags = 0x340000
(gdb) p /x loadURIOptions.mLoadFlags
$14 = 0x340000
(gdb) c
Continuing.
JavaScript error: resource://gre/modules/URIFixup.jsm, line 446: NS_ERROR_MALFORMED_URI: Couldn't build a valid uri
[Parent 24558: Unnamed thread 7f88002670]: D/SessionHistory Creating an active entry on nsDocShell 7f88b0c190 to test search (because we're showing an error page)

So I've edited the file to set sAllowKeyWordURL to be true:

$ git diff embedding/embedlite/embedshared/EmbedLiteViewChild.cpp
diff --git a/embedding/embedlite/embedshared/EmbedLiteViewChild.cpp
           b/embedding/embedlite/embedshared/EmbedLiteViewChild.cpp
index e06d68947ae9..5761689b6877 100644
--- a/embedding/embedlite/embedshared/EmbedLiteViewChild.cpp
+++ b/embedding/embedlite/embedshared/EmbedLiteViewChild.cpp
@@ -81,7 +81,7 @@ static struct {
     bool longTap;
 } sPostAZPCAsJson;
 
-static bool sAllowKeyWordURL = false;
+static bool sAllowKeyWordURL = true;
 
 static void ReadAZPCPrefs()
 {

This is in the C++ code, so I'll need to build it to test it. I've done a partial rebuild, but unfortunately that doesn't seem to have been enough. Given changing the variable using gdb and the partial build give the same results, I suspect there's more that needs changing.

Nevertheless, it's late and time to stop for the day, so I may as well run the build overnight to see whether that makes a difference. Just in case. I can then continue looking at this tomorrow.

If you'd like to catch up on all the diary entries, they're all available on my Gecko-dev Diary page.