1741694 - browser.search.search() doesn't work reliably with fission enabled

Status: RESOLVED WORKSFORME

(Core :: DOM: Content Processes, defect, P3)

Description

[robbendebiene]

With fission enabled (fission.autostart = true) the search engine API sometimes doesn't work.

Using the following manifest.json and background.js:

{
  "manifest_version": 2,
  "name": "Test",
  "version": "1",
  "permissions": [
    "search"
  ],
  "background": {
    "scripts": [ "background.js" ]
  },
  "browser_action": {
    "default_title": "Test"
  }
}

browser.browserAction.onClicked.addListener(async () => {
  const [ activeTab ] = await browser.tabs.query({active: true});
  
  const searches = await browser.search.get();
  
  const tab = await browser.tabs.create({
    openerTabId: activeTab.id,
    url: "about:blank",
    active: true
  });
	
  const randomSearchEngine = searches[Math.floor(Math.random()*searches.length)];
	
  browser.search.search({
    query: "Search query",
    engine: randomSearchEngine.name,
    tabId: tab.id
  });
});

When clicking the browser action the newly opened tab will sometimes stay blank instead of switching to the search query.

Comment 1

[Alex Cornestean]

Hello,

I reproduced the issue on the latest Nightly (96.0a1/20211118212756), Beta (95.0b9/20211118185700) and Release (94.0.1/20211103134640) under Windows 10 x64 and Ubuntu 16.04 LTS.

With fission.autostart = false, the first click on the browser action will yield a blank tab, however, every subsequent click on the browser action will open a tab with the search query.

With fission.autostart = true, each second click on the browser action will yield a blank tab i.e. 1st click=search query, 2nd click=blank tab, 3rd click=search query, 4th click=blank tab etc.

For more details, see the attached screenshots.

Comment 2

[Alex Cornestean]

Attachment: fission.autostart=false.png

Comment 3

[Alex Cornestean]

Attachment: fission.autostart=true.png

Comment 4

[Tomislav Jovanovic :zombie]

Hey Mark, can you please take a look into this?

Comment 5

[Mark Banner (:standard8)]

I believe this is tab browsing rather than search related.

As far as I can tell what gets passed through to openLinkIn is consistent and correct (called from here). It always has the correct search urls, and is indicating to load in the current tab.

If I add a breakpoint in ext-search.js, e.g. here, then the issue goes away - at least in the non-fission case.

This makes me think it is process switching related & timing. Since Fission is likely to be changing processes more often, this would seem to back up the idea.

I'm wondering if Mike might be able to look into this, or know who to pass it onto.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

This is really easy to reproduce by creating an add-on from the things in comment 0.

From what I can tell using rr, what's going on is that we attempt to load the search result page in a process that goes away soon after...and the new process that gets created tries to load about:blank.

So I think this is deeper DOM process switching stuff. I think I'm going to see if Nika has any idea what's going on here.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #6)

From what I can tell using rr, what's going on is that we attempt to load the search result page in a process that goes away soon after...and the new process that gets created tries to load about:blank.

If you can reproduce with rr, can you post a pernosco trace for me to look into? It would be useful for following it through.

We've got the process switching stuff down pretty well at this point, so it sounds like we're perhaps attempting to load the search result page and then some other code (perhaps extension code?) is cancelling the load during the process switch? An about:blank in the new process after the process switch which is quickly replaced is expected.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Sorry for the delay - Pernosco session is here: https://pernos.co/debug/3S2g3Kl1x83kcssQcqkvsg/index.html

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Nika, can you validate Mike's thoughts?

Comment 11

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9818a76ba62cc3608bb79a0f34945301826cafff&tochange=a7bfb6ebedc58c477df4dddf827821b316cabef3

Comment 12

[Jens Stutte [:jstutte]]

Would you mind verifying if the STR still apply? In case we should probably capture a new pernosco session, too.

Comment 13

[Artur Iunusov]

Managed to reproduce.
At least, I see this in the logs, when search is interrupted and page is blank:

"WebProgress Ignored: no longer current window global: file C:/Users/aiunu/mozilla-source/mozilla-unified/dom/ipc/BrowserParent.cpp:3141"

https://searchfox.org/mozilla-central/source/dom/ipc/BrowserParent.cpp#3141 ("The current process for this BrowsingContext may have changed since the notification was fired")

Comment 14

[Artur Iunusov]

Attachment: background_2.0.js

Comment 15

[Artur Iunusov]

However, the problem may be in the extension code itself. Because using the latest official MDN documentation I was able to create an extension that works like a charm (using disposition: "NEW_TAB"):

function search() {
    browser.search.get().then(function(searches) {
        const randomSearchEngine = searches[Math.floor(Math.random() * searches.length)];
        browser.search.search({
            query: "Search query",
            engine: randomSearchEngine.name,
            disposition: "NEW_TAB",
        });
    });
}
browser.browserAction.onClicked.addListener(search);

Comment 16

[Artur Iunusov]

Would this solution be enough? Because the search itself does not seem to be broken.

Comment 17

[robbendebiene]

Thanks for investigating the problem.
Using disposition: "NEW_TAB" might be a solution for some add-ons, but in Gesturefy the user can define the placement of the new tab
(see code). This is why the tabId is required. Back then when the API was first introduced this was the recommended way and also worked without any problems.

Comment 18

[Artur Iunusov]

Thanks for the feedback.
Got it. I will take a look into the problem closely, to figure out why the process goes away (or changes) for the new tabId

Comment 19

[Artur Iunusov]

I changed a bit your example. And it started working fine

browser.browserAction.onClicked.addListener(async () => {
  const activeTab  =  browser.tabs.getCurrent();
  
  const searches = await browser.search.get();
  
  const tab = await browser.tabs.create({
    openerTabId: activeTab.id,
    url: "about:blank",
    active: true
  });
	
  const randomSearchEngine = searches[Math.floor(Math.random()*searches.length)];
	
  browser.search.search({
    query: "Search query",
    engine: randomSearchEngine.name,
    tabId: tab.id
  });
});

(So, I am suspecting browser.tabs.query({active: true}) is causing issue)

Comment 20

[robbendebiene]

Perhaps it is important to note that for me the problem only occurred after having several tabs open. E.g. with less than 7 tabs everything worked but having more than 7 caused the problem. Therefore I thought it must be some sort of race condition.

Comment 21

[Artur Iunusov]

with browser.tabs.query({active: true}) every second click on extension leads to the blank page
with browser.tabs.getCurrent() it works fine in 100% cases

(I will take a look on rr traces and create a follow up bug)

Comment 22

[Artur Iunusov]

(still an issue)

sometimes browser.tabs.query({active: true}) does not work properly when fission is enabled

Comment 23

[Artur Iunusov]

I double checked everything, so

browser.tabs.query({active: true}) always returns correct tab
then, new tab is created

but search() sometimes does not work with that tab
My workaround is working, because openerTabId is always undefined (means, browser.tabs.getCurrent() returns undefined)

(checking regression window)

Comment 24

[Artur Iunusov]

renewed Pernosco session
https://pernos.co/debug/3S2g3Kl1x83kcssQcqkvsg/index.html

Comment 25

[Artur Iunusov]

Not reproducible anymore
This may be fixed due to recent improvements to about:blank handling.