1960620 - sync-about-blank: GeckoView:SetFocused and reftest:ContentReady are possibly racy

Status: RESOLVED DUPLICATE of bug 543435

(GeckoView :: General, defect, P2)

Description

[Vincent Hilla [:vhilla]]

I'm working on bug 543435 to load the initial about:blank synchronously. With this patch, layout reftests that rely on page focus are breaking on Android. I have some preliminary thoughts and require help finding a fix.

---

GeckoView has a handler GeckoView.onFocusChanged called from android.view.View.handleFocusGainInternal, which calls GeckoSession.setFocused and sends GeckoView:SetFocused to GeckoViewContent.sys.mjs. There, a browser element is focused. I think the message is sent quite early, but only once some content page starts loading and performs it's first event loop spin, the message is processed in the content.

I took a look at how layout reftests are executed, but am largely unfamiliar. There is some reftest.xhtml that excutes reftest-chrome.js, which calls reftest.sys.mjs OnRefTestLoad. This loads reftest-content.js in the content document, which waits for the initial about:blank load and calls OnInitialLoad. From this method, it sends reftest:ContentReady to the parent. The parent responds by doing InitAndStartRefTests and focuses a xul:browser element that is different from what GeckoViewContent.sys.mjs focused. It then starts loading tests, e.g. needs-focus.html and again loads reftest-content.js to be notified about the content load.

To better understand timing, I logged each event loop tick in GeckoViewContent.sys.mjs. On central, I observe that GeckoSession:setFocused is received on the first tick. reftest-content.js observes an about:blank load event around the 7th, the parent process receives a message about that on the 8th. So we'll first focus browser, then xul:browser.

With the patch from bug 543435, about:blank loads synchronously, so reftest:ContentReady is sent before the first event loop spin. The parent process first processes reftest:ContentReady, then GeckoSession:setFocused. This is confusing to me because these messages were sent in the opposite order, but maybe they were received by different execution contexts? Anyway, we end up with focus on browser. needs-focus.html starts loading, the parent calls updateBrowserRemotenessByURL, which leads to nsFrameLoaderOwner::UpdateFocusAndMouseEnterStateAfterFrameLoaderChange. Now, aOwner is the xul:browser browser['browser'].window['main-window'].#document @ 0x7974bbe08100 while the currently focused element is the browser browser.window['main-window'].#document @ 0x7974bbe08ac0. These aren't the same, so we don't SendActivate, don't move focus to the new document. The needs-focus.html reftest will call focus on an element, which will fail as the document doesn't have focus, causing the test to fail.

---

I'm unsure how to resolve this. A couple questions that would be helpful to me

• Is my understanding correct? Are there relevant aspects I missed? How about the process model?
• What is the difference between browser and xul:browser?
• If this is a race condition, how could it be resolved? Maybe delaying InitAndStartRefTests one tick?

Comment 1

[Vincent Hilla [:vhilla]]

Needinfo-ing you as I hope you might be able to help.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

What is the difference between browser and xul:browser?

None, if you're in the xul namespace you don't need to type the xul: bit. But maybe you're seeing two different browser elements?

You mention GeckoView:SetFocused but also GeckoSession:setFocused, which are different... Which one is it?

Now, aOwner is the xul:browser browser['browser'].window['main-window'].#document @ 0x7cae55908040 while the currently focused element is the browser browser['browser'].window['main-window'].#document @ 0x7cae55908040. These aren't the same, so we don't SendActivate [...]

But they are the same right? I'm not super familiar with the reftest setup on android, main-window comes from geckoview.xhtml, but presumably we are also loading reftest.xhtml, and it seems that your patch changed a bit the order of things so that reftest.xhtml is not the last thing to get focused. If so, yeah, making sure that it does get focus would be great, but ideally it shouldn't depend on ContentReady...

Maybe Tim recalls more about how this setup is supposed to work on Android?

Comment 3

[Vincent Hilla [:vhilla]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

You mention GeckoView:SetFocused but also GeckoSession:setFocused, which are different... Which one is it?

Sorry, I updated #c0. I mean the method GeckoSession.setFocused is sending a message "GeckoView:SetFocused".

Comment 4

[Vincent Hilla [:vhilla]]

And I meant to say the currently focused element is logged as browser.window['main-window'].#document, whereas the owner document that the element is compared to is browser['browser'].window['main-window'].#document. So they are different.

Comment 5

[Timothy Nikkel (:tnikkel)]

I haven't touch android reftest before. Maybe the code blame has some clues on who might know?

Comment 6

[Makoto Kato [:m_kato]]

Most differents of Desktop vs GeckoView are that UI thread isn't Gecko's main thread on GeckoView. Also, GeckoView provides Java APIs for delegation (that runs on UI thread). It means that browser side (written by Java or Kotlin. Fenix and android-componet) can hook loding pages and error pages. But Gecko doesn't consider that some XPCOMs runs by asynchronous, so GeckoView runs event spin loop for it to wait for result of delegation APIs.

"GeckoView:SetFocused" is dispatched from UI thread by window is activated etc. The event orders might be different for Gecko and GeckoView due to async code or event loop if delegation APIs such as navigation delegate are called.

GeckoView's window has one browser element only. You say document is different. How about main-window and browser? Same? Or It might be switching process?

What is the difference between browser and xul:browser?

Same. GeckoView creates browser by document.createXULElement("browser"));.

Comment 7

[Vincent Hilla [:vhilla]]

I'm pretty sure by now it's fine to delay InitAndStartRefTests on reftest:ContentReady by one tick. That way, we ensure the same order as on central, i.e. GeckoSession:setFocused is processed first.