flypig.co.uk

Today I'm continuing my investigation of an error that happens just after the user has granted permissions for a site to use the microphone. The problem generates an error to the console that looks like this:

JavaScript error: resource://gre/actors/AudioPlaybackParent.jsm, line 20: 
    TypeError: browser is null

As a quick recap, the line of code that's triggering the error, as it appears in the ESR 91 codebase, is the following:

    const browser = this.browsingContext.top.embedderElement;

This is a slight simplification of the code as it appears in ESR 78, like this:

    let topBrowsingContext = this.browsingContext.top;
    let browser = topBrowsingContext.embedderElement;

By looking at the history of this source file I was able to ascertain that the code has been merged into a single line but is otherwise the same, save for the fact that browser is now annotated with a const keyword.

The two variants of this code really appear to do very similar things, so what's interesting is that despite this change being barely different in terms of functionality, the error doesn't trigger under the same circumstances in ESR 78.

It seems unlikely that this const-ness is going to be the problem, but I suppose it's worth checking to make sure. The AudioPlaybackParent.jsm file is part of the gecko package and I don't want to have to rebuild everything, so I'm going to edit it in situ by accessing the omni.ja file.

cd omni/
./omni.sh unpack
find . -iname "AudioPlaybackParent.jsm" -exec vim {} \;
./omni.sh pack

I've made the following edits so that the version used is equivalent to the ESR 78 formulation:

  receiveMessage(aMessage) {
    //const browser = this.browsingContext.top.embedderElement;
    let topBrowsingContext = this.browsingContext.top;
    let browser = topBrowsingContext.embedderElement;
[...]

As expected, this has no practical effect, other than changing the line number the error is triggered on:

JavaScript error: resource://gre/actors/AudioPlaybackParent.jsm, line 22: 
    TypeError: browser is null

This means the error must be elsewhere. I've reversed the change I just made to avoid anything unexpected happening. But I now need to figure out why the browsing context is returning a null value for the embedderElement when, presumably, it didn't before.

Except, it turns out that's not quite the full story. While I thought the error didn't trigger at all in ESR 78, it seems it's not quite as simple as that. Earlier this morning I received the following messages from fellow Sailfish OS user and developer Mark Washeim (poetaster) on Mastodon suggesting otherwise:
   
This is fascinating. I can't seem to get it to trigger at all myself, but clearly it does under the right conditions. This was a really helpful intervention from Mark.

It nevertheless feels like something is different, so it's worth taking a look at the code involved. Here's the code in BrowsingContext for extracting the top window, taken from BrowsingContext.cpp:

BrowsingContext* BrowsingContext::Top() {
  BrowsingContext* bc = this;
  while (bc->mParentWindow) {
    bc = bc->GetParent();
  }
  return bc;
}

Since this can't reasonably be null here, the method is guaranteed to return a non-null value. But that's not our problem, our problem is the embedderElement hanging off this. Here's the code for this, taken this time from BrowsingContext.h

  // Get the embedder element for this BrowsingContext if the embedder is
  // in-process, or null if it's not.
  Element* GetEmbedderElement() const { return mEmbedderElement; }

There aren't any other immediately obvious differences in this between the two versions. When running the two versions of the browser the results are interesting. Not only is there no indication of this error on the console for ESR 78, but having granted permission to the site to use the microphone, I actually got to hear some resulting feedback. That doesn't happen on my ESR 91 build. We don't know whether this difference is related to the error though.

But while I don't know whether the error output to the console is the reason why the microphone is left inert on ESR 91, fixing nevertheless would seem like the right thing to do at this point.

So I'm going to have to follow that embedderElement variable down the rabbit hole to see where it leads.

None of the code related to embedderElement in the BrowsingContext class differs between ESR 78 and ESR 91. The external interface to it appears to be the SetEmbedderElement() method. This is quite a complex method and there's also the question of how it gets called. So I think some stepping through of the code will be in order to compare how ESR 78 is handling it with the ESR 91 flow.

To start off I've placed a breakpoint on BrowsingContext::SetEmbedderElemenet() to find out where the value actually gets set. But whether I do this on ESR 78 or ESR 91 I don't get a hit. Looking through the code I can't see any other way that the value can be set, but it's possible there's some way I missed. So I've tried to place a watch point on the variable instead.

The mEmbedderElement variable is a member of the BrowsingContext class, so the variable won't exist until an instance of the class has been constructed. So I've first placed a breakpoint on the BrowsingContext constructor. Once that's called the object will actually exist and I can then find the memory location that mEmbedderElement points to. I can then add a watch on that memory location, which should trigger a break whenever the variable is written to.

Now I know the value on ESR 91 remains null, so I'm particularly interested to find out where the value gets set on ESR 78:

(gdb) b BrowsingContext::BrowsingContext
(gdb) r
[...]
Thread 10 "GeckoWorkerThre" hit Breakpoint 1, mozilla::dom::
    BrowsingContext::BrowsingContext (this=this@entry=0x7f809a02a0, 
    aParentWindow=0x0,
    aGroup=0x7f80bef930, aBrowsingContextId=1, aType=mozilla::dom::
    BrowsingContext::Type::Content, aFields=...)
    at docshell/base/BrowsingContext.cpp:392
392     BrowsingContext::BrowsingContext(WindowContext* aParentWindow,
(gdb) n
92      ${PROJECT}/obj-build-mer-qt-xr/dist/include/nsWrapperCache.h: No such 
    file or directory.
(gdb) n
410           mUseRemoteSubframes(false) {
(gdb) print &mEmbedderElement
$1 = (RefPtr<mozilla::dom::Element> *) 0x7f809a04f8
(gdb) watch *0x7f809a04f8
Hardware watchpoint 2: *0x7f809a04f8
(gdb) c
Continuing.

This watchpoint is never hit. There are several reasons I can think of for why this might happen. I don't use watchpoints often, so one possibility is that I'm using it wrong. I've also noticed the debugger crashing out occasionally with this particular ESR 78 build, so it could also be an issue with the debugger. Finally it could simply be that the value never gets written to.

There is another way to check this, which is to find out the value it takes when it gets read. To test this I placed a breakpoint on GetEmbedderElement(). On ESR 91 the breakpoint is hit when I request the microphone permission. At that point it's null, as anticipated:

(gdb) b GetEmbedderElement
Breakpoint 3 at 0x7fbc79cbbc: GetEmbedderElement. (7 locations)
(gdb) r
Starting program: /usr/bin/sailfish-browser 
[...]
Thread 10 "GeckoWorkerThre" hit Breakpoint 3, mozilla::dom::
    BrowsingContext::GetEmbedderElement (this=0x7fb8bb2850)
    at ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/dom/
    BrowsingContext.h:335
335     ${PROJECT}/obj-build-mer-qt-xr/dist/include/mozilla/dom/
    BrowsingContext.h: No such file or directory.
(gdb) p mEmbedderElement
$3 = {mRawPtr = 0x0}
(gdb) 

On ESR 78 I don't have so much luck. The breakpoint won't stick to GetEmbedderElement() at all: apparently it doesn't exist as a debug symbol.

Okay, fair enough, there's another way to do this too, which is to print out the value from inside the AudioPlaybackParent::receiveMessage() method in the JavaScript. This is the method that's generating the error on ESR 91.

But when I do this, I find this JavaScript method is never hit either. It means that the fix may not be to ensure mEmbedderElement is set to a non-null value at all, but rather making sure that the AudioPlaybackParent::receiveMessage() is never called.

This would seem to tally with Mark's experiences. And alongside Mark's comment this gives me a slightly new direction to look in to. Unfortunately I don't have time to look in to this new direction today. That means that tomorrow I'll need to look at where the AudioPlaybackParent::receiveMessage() is getting called from.

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