flypig.co.uk

The build was running overnight and by the morning had completed its work. Here's a diff of the changes I made yesterday in the hope of fixing the Select widget crash:

$ git diff HEAD~~ HEAD~
diff --git a/embedding/embedlite/config/mozconfig.merqtxulrunner b/embedding/
    embedlite/config/mozconfig.merqtxulrunner
index 2593d32fd5a9..045996645139 100644
--- a/embedding/embedlite/config/mozconfig.merqtxulrunner
+++ b/embedding/embedlite/config/mozconfig.merqtxulrunner
@@ -3,6 +3,7 @@ export CFLAGS="-O3 -I/usr/include/freetype2"
 export CXXFLAGS="-O3 -I/usr/include/freetype2"
 export MOZ_DEBUG_SYMBOLS=1
 export MOZILLA_OFFICIAL=1
+export MOZ_USE_NATIVE_POPUP_WINDOWS=1
 
 mk_add_options PROFILE_GEN_SCRIPT=@TOPSRCDIR@/build/profile_pageloader.pl
 
diff --git a/embedding/embedlite/confvars.sh b/embedding/embedlite/confvars.sh
index 63a9f9be6a7e..985a6dd55290 100755
--- a/embedding/embedlite/confvars.sh
+++ b/embedding/embedlite/confvars.sh
@@ -16,6 +16,5 @@ MOZ_SERVICES_COMMON=
 MOZ_SERVICES_CRYPTO=
 MOZ_SERVICES_SYNC=
 MOZ_MEDIA_NAVIGATOR=1
-MOZ_USE_NATIVE_POPUP_WINDOWS=1
 MOZ_SERVICES_HEALTHREPORT=
 MOZ_DISABLE_EXPORT_JS=1

This is literally just moving the MOZ_USE_NATIVE_POPUP_WINDOWS variable from one build file to another. The aim is for it to be picked up, not by old-configure as before, but by toolkit/moz.configure following changes upstream. My hope is that with this change the pre-processor define of the same name will now be set again.

To test this out, I've run a partial build with the same inserted errors that I used for testing yesterday. And yes, this time the "correct" error fires:

In file included from Unified_cpp_layout_forms0.cpp:29:
${PROJECT}/gecko-dev/layout/forms/nsComboboxControlFrame.cpp:1615:2: error: 
    #error MOZ_USE_NATIVE_POPUP_WINDOWS=1
 #error MOZ_USE_NATIVE_POPUP_WINDOWS=1
  ^~~~~

That looks good. Moreover testing the browser shows that the crash is now gone. Hooray! That means there'll be time today to work on something else. You may recall that I'm working my way through the following four issues:

1. Single select widget.
2. External links.
3. Full screen.
4. Double-tap.

With the first now solved that means it's now time to tackle the second: "external links". The issue here is best demonstrated through the use of Jolla's URL scheme handling test page. The links on this page do a variety of things. Some open new pages, but many are intended to open external applications instead. For example, selecting a mailto: link should open the email app. Selecting an sms: link should open the SMS messaging app. And so on.

Currently only the internal page links are working and my task for today is to find out why the others are broken. Although selecting a link to open an external app doesn't work, pressing and holding on one brings up a popup window with the correct option provided, and which can be used to open an external app. So things are at least working there, as these screenshots show.
 
So I'm thinking that looking at files related to the pop-up window might be a good place to start. So first up I'm going to look into the Util.js source file from embedlite-components where we see a bunch of URI used in various ways like this:

  /*
   * URIs and schemes
   */

  makeURI: function makeURI(aURL, aOriginCharset, aBaseURI) {
    return Services.io.newURI(aURL, aOriginCharset, aBaseURI);
  },

  makeURLAbsolute: function makeURLAbsolute(base, url) {
    // Note:  makeURI() will throw if url is not a valid URI
    return this.makeURI(url, null, this.makeURI(base)).spec;
  },

  isLocalScheme: function isLocalScheme(aURL) {
    return ((aURL.indexOf("about:") == 0 &&
             aURL != "about:blank" &&
             aURL != "about:empty" &&
             aURL != "about:start") ||
            aURL.indexOf("chrome:") == 0);
  },

  isOpenableScheme: function isShareableScheme(aProtocol) {
    let dontOpen = /^(mailto|javascript|news|snews)$/;
    return (aProtocol && !dontOpen.test(aProtocol));
  },

  isShareableScheme: function isShareableScheme(aProtocol) {
    let dontShare = /^(chrome|about|file|javascript|resource)$/;
    return (aProtocol && !dontShare.test(aProtocol));
  },
[...]

Although a cursory skim of this file makes it look potentially relevant, I can't in fact find any direct link between this and the single-tap action. So I've moved on to the PopupOpener.qml file from embedlite-components. This file is the one that actually opens the pop we see in the screenshot.

        switch (topic) {
        case "Content:ContextMenu": root._openContextMenu(data); 
    break;
        case "embed:alert":         alert(data);    break;
        case "embed:confirm":       confirm(data);  break;
        case "embed:prompt":        prompt(data);   break;
        case "embed:login":         login(data);    break;
        case "embed:auth":          auth(data);     break;
        case "embed:permissions": {
            if (data.title === "geolocation") {
                permissions(data)
            } else {
                // Currently we don't support other permission requests.
                sendAsyncMessage("embedui:permissions", {
                    "allow": false,
                    "checkedDontAsk": false,
                    "id": data.id
                })
            }
            break
        }
        case "embed:webrtcrequest": webrtc(data);   break;
        case "embed:popupblocked":  blocked(data);  break;
        case "embed:select":        selector(data); break;
        }

From the contents of this file, written in QML and JavaScript, we can see the pop up is triggered by the Content:ContextMenu message. This, in turn, is sent from the ContextMenuHandler.js file in embedlite-components:

  /*
   * _processPopupNode - Generate and send a Content:ContextMenu message
   * to browser detailing the underlying content types at this.popupNode.
   * Note the event we receive targets the sub frame (if there is one) of
   * the page.
   */
  _processPopupNode: function _processPopupNode(aPopupNode, aX, aY, aInputSrc) {
    if (!aPopupNode)
      return;

    let { targetWindow: targetWindow,
          offsetX: offsetX,
          offsetY: offsetY } =
      Util.translateToTopLevelWindow(aPopupNode);

    let popupNode = this.popupNode = aPopupNode;
    let imageUrl = "";

    let state = {
      types: [],
      label: "",
      linkURL: "",
      linkTitle: "",
      linkProtocol: null,
      mediaURL: "",
      contentType: "",
      contentDisposition: "",
      string: "",
      visualViewport: {
          offsetLeft: 0,
          offsetTop: 0
      }
    };
[...]

    sendAsyncMessage("Content:ContextMenu", state);
  },

This looks promising, so worth digging a little deeper into. The ContextMenuHandler code is called indirectly by the event handler code in the same file:

  handleEvent: function ch_handleEvent(aEvent) {
    switch (aEvent.type) {
      case "contextmenu":
        this._onContentContextMenu(aEvent);
        break;
      case "pagehide":
        this.reset();
        break;
    }
  },

So now we're looking for the code that sends out the contextmenu event. This turns out to happen in the embedhelper.js file:

  _sendContextMenuEvent: function _sendContextMenuEvent(aElement, aX, aY) {
    let window = aElement.ownerDocument.defaultView;
    try {
      let cwu = window.windowUtils;
      cwu.sendMouseEventToWindow("contextmenu", aX, aY, 2, 1, 0, 
    false);
    } catch(e) {
      Cu.reportError(e);
    }
  },

The embedhelper.js file is another good place to dig further into. Not only is it responsible for opening up the context menu on long-press, it's also responsible for handling touch events on the page. That means it might tell us something about what happens when you select a link, not with a long press, but with a tap:

  receiveMessage: function receiveMessage(aMessage) {
    switch (aMessage.name) {
      case "Gesture:ContextMenuSynth": {
        let [x, y] = [aMessage.json.x, aMessage.json.y];
        let element = this._touchElement;
        this._sendContextMenuEvent(element, x, y);
        break;
      }
      case "Gesture:SingleTap": {
        if (SelectionHandler.isActive) {
            SelectionHandler._onSelectionCopy({xPos: aMessage.json.x, yPos: 
    aMessage.json.y});
        }

        try {
          let [x, y] = [aMessage.json.x, aMessage.json.y];
          this._sendMouseEvent("mousemove", content, x, y);
          this._sendMouseEvent("mousedown", content, x, y);
          this._sendMouseEvent("mouseup",   content, x, y);
        } catch(e) {
          Cu.reportError(e);
        }

        if (this._touchEventDefaultPrevented) {
          this._touchEventDefaultPrevented = false;
        } else {
          let uri = this._getLinkURI(this._touchElement);
          if (uri && (uri instanceof Ci.nsIURI)) {
            try {
              let winId = Services.embedlite.getIDByWindow(content);
              Services.embedlite.sendAsyncMessage(winId, "embed:
    linkclicked",
                                                  JSON.stringify({
                                                                   
    "uri": uri.asciiSpec
                                                                 }));
            } catch (e) {
              Logger.warn("embedhelper: sending async message 
    failed", e)
            }
          }
          this._touchElement = null;
        }
        break;
      }

Well, that looked like it was going to help, but after all that digging I can't see where it goes from there. However, as I was digging through all these files I did notice a few names repeatedly appearing that looked relevant. So this hasn't been entirely wasted effort. In particular I notice the phrase "protocol handler" coming up frequently. Searching through the entire project for this I find some more code that looks like it's relevant to what we need. There's even an interface defined in the nsIExternalProtocolService.idl file with the following method signature:

  /**
   * Retrieve the handler for the given protocol.  If neither the application
   * nor the OS knows about a handler for the protocol, the object this method
   * returns will represent a default handler for unknown content.
   *
   * @param aProtocolScheme the scheme from a URL: http, ftp, mailto, etc.
   *
   * Note: aProtocolScheme should not include a trailing colon, which is part
   * of the URI syntax, not part of the scheme itself (i.e. pass 
    "mailto" not
   * "mailto:").
   *
   * @return the handler, if any; otherwise a default handler
   */
  nsIHandlerInfo getProtocolHandlerInfo(in ACString aProtocolScheme);

This is just an abstract interface and what I really what to find is the code that implements it. Searching for this throws up lots of uses, but distinguishing which concrete implementation we want doesn't turn out to be so easy. So I've stuck a breakpoint on it and executed the code to find out. Here's the backtrace from when one the methods is hit after selecting one of the links on the test page. This is from ESR 91:

Thread 10 "GeckoWorkerThre" hit Breakpoint 1, 
    nsExternalHelperAppService::GetProtocolHandlerInfo (this=0x7fb8228270, 
    aScheme=..., 
    aHandlerInfo=0x7fde7d6530)
    at uriloader/exthandler/nsExternalHelperAppService.cpp:1179
1179        const nsACString& aScheme, nsIHandlerInfo** aHandlerInfo) {
(gdb) bt
#0  nsExternalHelperAppService::GetProtocolHandlerInfo (this=0x7fb8228270, 
    aScheme=..., aHandlerInfo=0x7fde7d6530)
    at uriloader/exthandler/nsExternalHelperAppService.cpp:1179
#1  0x0000007ff225c364 in nsExternalHelperAppService::LoadURI (
    this=0x7fb8228270, aURI=<optimized out>, aTriggeringPrincipal=0x7fb90c2f30, 
    aRedirectPrincipal=0x0, aBrowsingContext=0x7fb82237d0, 
    aTriggeredExternally=<optimized out>)
    at ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsCOMPtr.h:1363
#2  0x0000007ff493bf90 in nsDocShell::OnLinkClickSync (this=0x7fb8abd300, 
    aContent=0x7fb91454b0, aLoadState=0x7efc013a30, 
    aNoOpenerImplied=<optimized out>, aTriggeringPrincipal=<optimized out>)
    at ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/RefPtr.h:289
#3  0x0000007ff493c7e4 in OnLinkClickEvent::Run (this=0x7fb8960710)
    at ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsCOMPtr.h:859
#4  0x0000007ff19a3d00 in mozilla::RunnableTask::Run (this=0x555634ebd0)
    at ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/RefPtr.h:313
[...]
#26 0x0000007fef54889c in ?? () from /lib64/libc.so.6
(gdb) 

Although GetProtocolHandlerInfo() is the one with the breakpoint on, to my mind the interesting method is going to be the LoadURI() call that calls it in frame #1. Let's step through the code in this calling method to find out what it's doing; first on ESR 91:

Thread 10 "GeckoWorkerThre" hit Breakpoint 3, 
    nsExternalHelperAppService::LoadURI (this=0x7fb8228270, aURI=0x7fb90776b0,
    aTriggeringPrincipal=0x7fb90c2f30, aRedirectPrincipal=0x0, 
    aBrowsingContext=0x7fb82237d0, aTriggeredExternally=false)
    at uriloader/exthandler/nsExternalHelperAppService.cpp:
1014
1014                                        bool aTriggeredExternally) {
(gdb) n
1015      NS_ENSURE_ARG_POINTER(aURI);
(gdb) n
1017      if (XRE_IsContentProcess()) {
(gdb) n
1024      nsCOMPtr<nsIURI> escapedURI;
(gdb) n
1028      nsAutoCString scheme;
(gdb) n
1029      escapedURI->GetScheme(scheme);
(gdb) n
1030      if (scheme.IsEmpty()) return NS_OK;  // must have a scheme
(gdb) n
1033      nsAutoCString externalPref(kExternalProtocolPrefPrefix);
(gdb) n
1034      externalPref += scheme;
(gdb) n
1035      bool allowLoad = false;
(gdb) n
1038        if (NS_FAILED(
(gdb) n
1044      if (!allowLoad) {
(gdb) p allowLoad
$1 = true
(gdb) n
1053      if (aBrowsingContext && aTriggeringPrincipal &&
(gdb) n
1054          !StaticPrefs::security_allow_disjointed_external_uri_loads() &&
(gdb) n
434     in ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/BasePrincipal.h
(gdb) n
1059        RefPtr<BrowsingContext> bc = aBrowsingContext;
(gdb) n
1060        WindowGlobalParent* wgp = bc->Canonical()->GetCurrentWindowGlobal();
(gdb) n
1065        if (bc->IsTop() && !bc->HadOriginalOpener() && wgp) {
(gdb) n
1066          RefPtr<nsIURI> uri = wgp->GetDocumentURI();
(gdb) n
1068              uri && uri->GetSpecOrDefault().EqualsLiteral(&quot;about:
    blank&quot;);
(gdb) n
1066          RefPtr<nsIURI> uri = wgp->GetDocumentURI();
(gdb) n
1071        while (!foundAccessibleFrame) {
(gdb) n
1072          if (wgp) {
(gdb) n
1074                aTriggeringPrincipal->Subsumes(wgp->DocumentPrincipal());
(gdb) n
1078          BrowsingContext* parent = bc->GetParent();
(gdb) n
1079          if (!parent) {
(gdb) p parent
$3 = (mozilla::dom::BrowsingContext *) 0x0
(gdb) n
1086        if (!foundAccessibleFrame) {
(gdb) n
1099        if (!foundAccessibleFrame) {
(gdb) n
1059        RefPtr<BrowsingContext> bc = aBrowsingContext;
(gdb) n
1104      nsCOMPtr<nsIHandlerInfo> handler;
(gdb) n
1109          do_CreateInstance(&quot;@mozilla.org/
    content-dispatch-chooser;1&quot;, &rv);
(gdb) n
1110      NS_ENSURE_SUCCESS(rv, rv);
(gdb) n
1112      return chooser->HandleURI(
(gdb) n
1109          do_CreateInstance(&quot;@mozilla.org/
    content-dispatch-chooser;1&quot;, &rv);
(gdb)

Nothing looks particularly unreasonable here. The code executes all the way to the end of the method and calls HandleURI() at the end. There's no early return and the handler seems to be picked up correctly. Let's compare this against ESR 78 to see if there are any differences:

Thread 10 "GeckoWorkerThre" hit Breakpoint 2, 
    nsExternalHelperAppService::LoadURI (this=0x7f80b49c50, aURI=0x7f80b6bc30, 
    aTriggeringPrincipal=0x7f80ef4ba0, aBrowsingContext=0x7f80bf24c0)
    at uriloader/exthandler/nsExternalHelperAppService.cpp:936
936                                         BrowsingContext* aBrowsingContext) {
(gdb) n
937       NS_ENSURE_ARG_POINTER(aURI);
(gdb) n
939       if (XRE_IsContentProcess()) {
(gdb) n
948       if (spec.Find("%00") != -1) return NS_ERROR_MALFORMED_URI;
(gdb) n
950       spec.ReplaceSubstring("\"", "%22");
(gdb) n
951       spec.ReplaceSubstring("`", "%60");
(gdb) n
953       nsCOMPtr<nsIIOService> ios(do_GetIOService());
(gdb) n
954       nsCOMPtr<nsIURI> uri;
(gdb) n
955       nsresult rv = ios->NewURI(spec, nullptr, nullptr, getter_AddRefs(
    uri));
(gdb) n
959       uri->GetScheme(scheme);
(gdb) n
960       if (scheme.IsEmpty()) return NS_OK;  // must have a scheme
(gdb) n
963       nsAutoCString externalPref(kExternalProtocolPrefPrefix);
(gdb) n
964       externalPref += scheme;
(gdb) n
965       bool allowLoad = false;
(gdb) n
966       if (NS_FAILED(Preferences::GetBool(externalPref.get(), &allowLoad))) {
(gdb) n
968         if (NS_FAILED(
(gdb) n
974       if (!allowLoad) {
(gdb) p allowLoad
$1 = true
(gdb) n
983       if (aBrowsingContext && aTriggeringPrincipal &&
(gdb) n
984           !StaticPrefs::security_allow_disjointed_external_uri_loads() &&
(gdb) n
1013      handler->GetPreferredAction(&preferredAction);
(gdb) n
1015      handler->GetAlwaysAskBeforeHandling(&alwaysAsk);
(gdb) n
1019      if (!alwaysAsk && (preferredAction == nsIHandlerInfo::useHelperApp ||
(gdb) n
1021        rv = handler->LaunchWithURI(uri, aBrowsingContext);
(gdb) n
1025        if (rv != NS_ERROR_FILE_NOT_FOUND) {
(gdb) p rv
$3 = nsresult::NS_OK
(gdb) n
1008      nsCOMPtr<nsIHandlerInfo> handler;
(gdb) 

There are definite differences here, but honestly it's not clear whether these are due to changes in the code between the two versions, or differences in the state. What is interesting, however, is that when stepping over the LaunchWithURI() method on ESR 78 the external Messaging app opens for me to enter an SMS message. That means that we're definitely in the right ballpark of a location here: it must be the LaunchWithURI() call that's opening the external link. This gets called on ESR 78, but it's not getting called on ESR 91.

Despite the differences between the two pieces of code, one area that separates the two execution flows is that on ESR 78 the following condition is false and the block it encloses is skipped:

(aBrowsingContext && aTriggeringPrincipal &&
      !StaticPrefs::security_allow_disjointed_external_uri_loads() &&
      !aTriggeringPrincipal->IsSystemPrincipal())

On ESR 91 the condition is slightly different, but unlike on ESR 78 the condition is true and the associated block is entered into as a result. Here's the condition on ESR 91:

(aBrowsingContext && aTriggeringPrincipal &&
      !StaticPrefs::security_allow_disjointed_external_uri_loads() &&
      // Add-on principals are always allowed:
      !BasePrincipal::Cast(aTriggeringPrincipal)->AddonPolicy() &&
      // As is chrome code:
      !aTriggeringPrincipal->IsSystemPrincipal())

I'm going to try to find out why one is entered and the other isn't. So on ESR 78 I've executed the code right up to just prior to the condition being checked and now have execution paused. I can now try to check the values that make up this condition. Here's the result on ESR 78:

(gdb) p aBrowsingContext
$4 = (mozilla::dom::BrowsingContext *) 0x7f80bf24c0
(gdb) p aTriggeringPrincipal
$5 = (nsIPrincipal *) 0x7f80ef4ba0
(gdb) p StaticPrefs::security_allow_disjointed_external_uri_loads()
No symbol "security_allow_disjointed_external_uri_loads" in namespace 
    "mozilla::StaticPrefs".
(gdb) p aTriggeringPrincipal->IsSystemPrincipal()
Cannot evaluate function -- may be inlined
(gdb) p aTriggeringPrincipal->mKind
There is no member or method named mKind.
(gdb) p ((BasePrincipal*)aTriggeringPrincipal)->mKind
$6 = mozilla::BasePrincipal::eContentPrincipal
(gdb) 

On ESR 78 this is going to convert our condition in to the following, which will clearly resolve to false:

(true && true &&
      !true &&
      !false)

On ESR 91 things are a little more complex:

(gdb) p aBrowsingContext
$4 = (mozilla::dom::BrowsingContext *) 0x7fb82237d0
(gdb) p aTriggeringPrincipal
$5 = (nsIPrincipal *) 0x7fb90c2f30
(gdb) p StaticPrefs::security_allow_disjointed_external_uri_loads()
No symbol "security_allow_disjointed_external_uri_loads" in namespace 
    "mozilla::StaticPrefs".
(gdb) p ((ContentPrincipal*)aTriggeringPrincipal)->mAddon
$6 = {<mozilla::detail::MaybeStorage<mozilla::WeakPtr<mozilla::extensions::
    WebExtensionPolicy, (mozilla::detail::WeakPtrDestructorBehavior)0>, false>> 
    = {<mozilla::detail::MaybeStorageBase<mozilla::WeakPtr<mozilla::extensions::
    WebExtensionPolicy, (mozilla::detail::WeakPtrDestructorBehavior)0>, false>> 
    = {
      mStorage = {val = {mRef = {mRawPtr = 0x7fb90b0710}}}}, 
    mIsSome = 1 '\001'}, <mozilla::detail::Maybe_CopyMove_Enabler<mozilla::
    WeakPtr<mozilla::extensions::WebExtensionPolicy, (mozilla::detail::
    WeakPtrDestructorBehavior)0>, false, true, true>> = {<No data fields>}, <No 
    data fields>}
(gdb) p ((ContentPrincipal*)aTriggeringPrincipal)->mAddon.mStorage.val
$7 = {mRef = {mRawPtr = 0x7fb90b0710}}
(gdb) p ((ContentPrincipal*)aTriggeringPrincipal)->mAddon.mIsSome
$8 = 1 '\001'
(gdb) p ((BasePrincipal*)aTriggeringPrincipal)->mKind
$9 = mozilla::BasePrincipal::eContentPrincipal
(gdb) 

Fitting these values into the condition on ESR 91 gives us the following:

(true && true &&
      true &&
      // Add-on principals are always allowed:
      !BasePrincipal::Cast(aTriggeringPrincipal)->AddonPolicy() &&
      // As is chrome code:
      !false)

I wasn't able to cleanly determine the value of AddonPolicy() using the debugger, but since the condition is passing we know that it must return false in order for the overall condition to resolve to true (which it does).

This is a very clear difference, but in practice it doesn't seem to be the relevant part that we need. Even if this condition is entered in to the code still ends up executing to the end of the method where the HandleURI() method is called. And that's the bit that should be triggering the Messaging app to open.

Still, the investigation has led us to the relevant place, that much we know for sure. So we can change tack slightly and take a look at the history for this bit of code. It's changed quite a bit between ESR 78 and ESR 91 and it might be helpful to understand why. Thankfully we have the tools for this in the form of git blame and git diff. First I want to find out the relevant changes. There are quite a few and I have to go back through a bunch of commits that just perform reformatting of the code, but eventually I get to this point:

$ git blame 3e36bc378fca8~ uriloader/exthandler/nsExternalHelperAppService.cpp \
    -L 1110,1119
Blaming lines:   0% (10/3132), done.
ca6da5a72eddc (sdwilsh@shawnwilsher.com 2007-07-25 21:24:25 -0700 1110)   
    nsCOMPtr<nsIHandlerInfo> handler;
ca6da5a72eddc (sdwilsh@shawnwilsher.com 2007-07-25 21:24:25 -0700 1111)
    rv = GetProtocolHandlerInfo(scheme, getter_AddRefs(handler));
ca6da5a72eddc (sdwilsh@shawnwilsher.com 2007-07-25 21:24:25 -0700 1112)   
    NS_ENSURE_SUCCESS(rv, rv);
430d42c69d3d8 (dveditz%cruzio.com       2004-10-25 07:46:01 +0000 1113) 
ca6da5a72eddc (sdwilsh@shawnwilsher.com 2007-07-25 21:24:25 -0700 1114)   
    nsCOMPtr<nsIContentDispatchChooser> chooser =
265e6721798a4 (Sylvestre Ledru          2018-11-30 11:46:48 +0100 1115)       
    do_CreateInstance(&quot;@mozilla.org/content-dispatch-chooser;1&quot;, &rv);
ca6da5a72eddc (sdwilsh@shawnwilsher.com 2007-07-25 21:24:25 -0700 1116)   
    NS_ENSURE_SUCCESS(rv, rv);
265e6721798a4 (Sylvestre Ledru          2018-11-30 11:46:48 +0100 1117) 
84589d971b4d4 (pbz                      2020-10-29 13:43:46 +0000 1118)   
    return chooser->HandleURI(handler, uri, aTriggeringPrincipal,
8002a3c48cde6 (Gijs Kruitbosch          2021-02-22 19:00:10 +0000 1119)         
                        aBrowsingContext, aTriggeredExternally);

The relevant change is on the penultimate line here: commit 84589d971b4d4. Let's take a look at the diff to the file between the two versions (before and after the commit) in full:

$ git diff 84589d971b4d4~ 84589d971b4d4 uriloader/exthandler/
    nsExternalHelperAppService.cpp
diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/
    exthandler/nsExternalHelperAppService.cpp
index 40b7a00bf957..59ace07f55af 100644
--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
@@ -1047,30 +1047,12 @@ nsExternalHelperAppService::LoadURI(nsIURI* aURI,
   rv = GetProtocolHandlerInfo(scheme, getter_AddRefs(handler));
   NS_ENSURE_SUCCESS(rv, rv);
 
-  nsHandlerInfoAction preferredAction;
-  handler->GetPreferredAction(&preferredAction);
-  bool alwaysAsk = true;
-  handler->GetAlwaysAskBeforeHandling(&alwaysAsk);
-
-  // if we are not supposed to ask, and the preferred action is to use
-  // a helper app or the system default, we just launch the URI.
-  if (!alwaysAsk && (preferredAction == nsIHandlerInfo::useHelperApp ||
-                     preferredAction == nsIHandlerInfo::useSystemDefault)) {
-    rv = handler->LaunchWithURI(uri, aBrowsingContext);
-    // We are not supposed to ask, but when file not found the user most likely
-    // uninstalled the application which handles the uri so we will continue
-    // by application chooser dialog.
-    if (rv != NS_ERROR_FILE_NOT_FOUND) {
-      return rv;
-    }
-  }
-
   nsCOMPtr<nsIContentDispatchChooser> chooser =
       do_CreateInstance(&quot;@mozilla.org/content-dispatch-chooser;1&quot;, 
    &rv);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  return chooser->Ask(handler, uri, aTriggeringPrincipal, aBrowsingContext,
-                      nsIContentDispatchChooser::REASON_CANNOT_HANDLE);
+  return chooser->HandleURI(handler, uri, aTriggeringPrincipal,
+                            aBrowsingContext);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
    ///////////////////////

Now we're getting somewhere. This is where the code switched from the version in ESR 78 to the one we now see in ESR 91. As this shows, the LaunchWithURI() method that's doing the work on ESR 78 has been removed in the ESR 91 version. What's the reason for this? Let's check the commit message to find out.

$ git log -1 84589d971b4d4
commit 84589d971b4d419492d131a70874e5b92e3d5463
Author: pbz <pbz@mozilla.com>
Date:   Thu Oct 29 13:43:46 2020 +0000

    Bug 1565574 - Added permission required to open external protocol handlers. 
    r=Gijs
    
    - Added pref to toggle permission feature
    - Updated ContentDispatchChooser to check for permission and  manage a 
    multi dialog flow.
    
    Differential Revision: https://phabricator.services.mozilla.com/D92945

So it seems there have been changes to support opening a dialog window, rather than directly opening the URLs in a separate application. I'm not going to paste it here, but it's worth having a look at the full changeset to see what's going on there. It shows how much of the code has been moved out of the C++ file and into the JavaScript ContentDispatchChooser.jsm file. We can even see that the call to launchWithURI() has been moved there.

To find out what's going on I've annotated the code with some lines for generating debug output. I apologies for the lengthy code dump here, but in order to understand how the debug output relates to the execution flow it's essential to see where I've placed the calls to dump() that are generating them. So here it is. This is the code form ESR 91, identical to the original apart from the additional debug output lines that I've added:

  /**
   * Prompt the user to open an external application.
   * If the triggering principal doesn't have permission to open apps for the
   * protocol of aURI, we show a permission prompt first.
   * If the caller has permission and a preferred handler is set, we skip the
   * dialogs and directly open the handler.
   * @param {nsIHandlerInfo} aHandler - Info about protocol and handlers.
   * @param {nsIURI} aURI - URI to be handled.
   * @param {nsIPrincipal} [aPrincipal] - Principal which triggered the load.
   * @param {BrowsingContext} [aBrowsingContext] - Context of the load.
   * @param {bool} [aTriggeredExternally] - Whether the load came from outside
   * this application.
   */  
  async handleURI(
    aHandler,
    aURI,
    aPrincipal,
    aBrowsingContext,
    aTriggeredExternally = false
  ) { 
    let callerHasPermission = this._hasProtocolHandlerPermission(
      aHandler.type,
      aPrincipal
    );
    dump("HANDLE: handleURI type: " + aHandler.type + "\n");

    // Force showing the dialog for links passed from outside the application.
    // This avoids infinite loops, see bug 1678255, bug 1667468, etc.
    if (
      aTriggeredExternally &&
      gPrefs.promptForExternal &&
      // ... unless we intend to open the link with a website or extension:
      !(  
        aHandler.preferredAction == Ci.nsIHandlerInfo.useHelperApp &&
        aHandler.preferredApplicationHandler instanceof Ci.nsIWebHandlerApp
      )   
    ) { 
      dump("HANDLE: always ask\n");
      aHandler.alwaysAskBeforeHandling = true;
    }   

    // Skip the dialog if a preferred application is set and the caller has 
    // permission.
    if (
      callerHasPermission &&
      !aHandler.alwaysAskBeforeHandling &&
      (aHandler.preferredAction == Ci.nsIHandlerInfo.useHelperApp ||
        aHandler.preferredAction == Ci.nsIHandlerInfo.useSystemDefault)
    ) { 
      dump("HANDLE: lanchWithURI internal\n");
      try {
        aHandler.launchWithURI(aURI, aBrowsingContext);
      } catch (error) {
        dump("HANDLE: error\n");
        // We are not supposed to ask, but when file not found the user most 
    likely
        // uninstalled the application which handles the uri so we will continue
        // by application chooser dialog.
        if (error.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
          aHandler.alwaysAskBeforeHandling = true;
        } else {
          throw error;
        }   
      }   
    }   

    // We will show a prompt, record telemetry.
    try {
      dump("HANDLE: show prompt\n");
      ContentDispatchChooserTelemetry.recordTelemetry(
        aHandler.type,
        aBrowsingContext,
        aPrincipal
      );  
    } catch (error) {
      dump("HANDLE: report error show prompt\n");
      Cu.reportError(error);
    }   

    let shouldOpenHandler = false;
    try {
      shouldOpenHandler = await this._prompt(
        aHandler,
        aPrincipal,
        callerHasPermission,
        aBrowsingContext
      );  
    } catch (error) {
      dump("HANDLE: report error prompt\n");
      Cu.reportError(error.message);
    }   

    dump("HANDLE: shouldOpenHandler: " + shouldOpenHandler + 
    "\n");
    if (!shouldOpenHandler) {
      return;
    }

    dump("HANDLE: lanchWithURI end\n");
    // Site was granted permission and user chose to open application.
    // Launch the external handler.
    aHandler.launchWithURI(aURI, aBrowsingContext);
  }

Now when I execute the browser and press on the SMS link on the test page I get the following debug output appearing in the console:

HANDLE: handleURI type: sms
HANDLE: show prompt
HANDLE: report error prompt
HANDLE: shouldOpenHandler: false

This is really useful. It tells us that the code is reaching the condition on shouldOpenHandler. Since this is set to false the method returns early at that point, and hence the launchWithURI() call right at the end of the method is never reached. It looks like the code is calling a handler related to opening a popup, which the Sailfish browser isn't set up to provide yet. Because of this the call to this._prompt() is returning false. We want it to return true, but while I'm still testing things I can force it to return true to see whether this fixes things. This change will guarantee that the launchWithURI() call at the end of the method is executed.

And indeed it is, but rather than opening the SMS app it instead generates the following error:

JavaScript error: resource://gre/modules/ContentDispatchChooser.jsm, line 339: 
    NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (
    NS_ERROR_ILLEGAL_VALUE) [nsIHandlerInfo.launchWithURI]

How to fix this? I still need to fix the prompting issue, but I'm going to put that on hold until I've dealt with this error. The rabbit hole continues to draw me further in, this time with the focus on the aHandler object associated with the launchWithURI() method. Since this is being passed in at the top of the method and so is coming from the C++, it means I can use the debugger to tell us a little more about it. Here's what the handler object provides:

(gdb) ptype (nsIHandlerInfo*)handler.mRawPtr
type = class nsIHandlerInfo : public nsISupports {
  public:
    virtual nsresult GetType(nsACString &);
    virtual nsresult GetDescription(nsAString &);
    virtual nsresult SetDescription(const nsAString &);
    virtual nsresult GetPreferredApplicationHandler(nsIHandlerApp **);
    virtual nsresult SetPreferredApplicationHandler(nsIHandlerApp *);
    virtual nsresult GetPossibleApplicationHandlers(nsIMutableArray **);
    virtual nsresult GetHasDefaultHandler(bool *);
    virtual nsresult GetDefaultDescription(nsAString &);
    virtual nsresult LaunchWithURI(nsIURI *, mozilla::dom::BrowsingContext *);
    virtual nsresult GetPreferredAction(nsHandlerInfoAction *);
    virtual nsresult SetPreferredAction(nsHandlerInfoAction);
    virtual nsresult GetAlwaysAskBeforeHandling(bool *);
    virtual nsresult SetAlwaysAskBeforeHandling(bool);
} *
(gdb) 

Searching for some of these methods in the code throws up the fact that we have a couple of patches related to this. These are patch 0056 "Use libcontentaction for custom scheme uri handling" and patch 0062 "Fix content action integration to work". Both of these are substantial patches which clearly relate to how the external protocol handling is supported. So my thought at this point is that I should try to apply these patches to get the underlying content action code in place before working on the other issues I've discovered. Thankfully, the patches apply cleanly on first attempt, which is unexpected:

$ git am --3way \
    ../rpm/0056-sailfishos-gecko-Use-libcontentaction-for-custom-sch.patch
Applying: Use libcontentaction for custom scheme uri handling. JB#47892
Using index info to reconstruct a base tree...
M       old-configure.in
M       uriloader/exthandler/unix/nsOSHelperAppService.cpp
M       xpcom/io/moz.build
M       xpcom/io/nsLocalFileUnix.cpp
Falling back to patching base and 3-way merge...
Auto-merging xpcom/io/nsLocalFileUnix.cpp
Auto-merging xpcom/io/moz.build
Auto-merging uriloader/exthandler/unix/nsOSHelperAppService.cpp
Auto-merging old-configure.in
$ git am --3way \
    ../rpm/0062-sailfishos-contentaction-Fix-content-action-integrat.patch
Applying: Fix content action integration to work. Fixes JB#51235
Using index info to reconstruct a base tree...
M       uriloader/exthandler/moz.build
M       uriloader/exthandler/unix/nsOSHelperAppService.cpp
M       xpcom/io/nsLocalFileUnix.cpp
Falling back to patching base and 3-way merge...
Auto-merging xpcom/io/nsLocalFileUnix.cpp
Auto-merging uriloader/exthandler/unix/nsOSHelperAppService.cpp
Auto-merging uriloader/exthandler/moz.build

There are significant changes to the C++ code caused by applying these patches, so to test them I'll need to rebuild the library. As always, that means this is the most suitable place to stop for today. It'll take until tomorrow for the rebuild to complete at which point I can run the same tests again to see if anything has changed. That'll be my task for tomorrow morning.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.