1808630 - Intermittent TV [tier 2] Assertion failure: Group() == aTarget->Group() (A BrowsingContext should never see a context from a different group), at docshell/base/BrowsingContext.cpp:1357 in toolkit/content/tests/chrome/test_edit_contextmenu.html

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P5)

Description

[Treeherder Bug Filer]

Filed by: mlaza [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=401376187&repo=autoland
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/ff4AHXkeTM6-LJqJErLFlA/runs/0/artifacts/public/logs/live_backing.log
Reftest URL: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/ff4AHXkeTM6-LJqJErLFlA/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

---

[task 2023-01-04T19:56:48.097Z] 19:56:48     INFO - TEST-START | toolkit/content/tests/chrome/test_edit_contextmenu.html
[task 2023-01-04T19:56:48.098Z] 19:56:48     INFO - GECKO(764) | [Parent 6424, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005 (NS_ERROR_FAILURE): file /builds/worker/checkouts/gecko/chrome/nsChromeRegistry.cpp:180
[task 2023-01-04T19:56:48.099Z] 19:56:48     INFO - GECKO(764) | [Parent 6424, Main Thread] WARNING: 'NS_FAILED(rv)', file /builds/worker/checkouts/gecko/chrome/nsChromeProtocolHandler.cpp:73
[task 2023-01-04T19:56:48.110Z] 19:56:48     INFO - GECKO(764) | [Parent 6424, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /builds/worker/checkouts/gecko/parser/html/nsHtml5StreamParser.cpp:1234
[task 2023-01-04T19:56:48.160Z] 19:56:48     INFO - GECKO(764) | Assertion failure: Group() == aTarget->Group() (A BrowsingContext should never see a context from a different group), at /builds/worker/checkouts/gecko/docshell/base/BrowsingContext.cpp:1357
[task 2023-01-04T19:56:48.340Z] 19:56:48     INFO -  Initializing stack-fixing for the first stack frame, this may take a while...
[task 2023-01-04T19:57:08.868Z] 19:57:08     INFO - GECKO(764) | #01: mozilla::dom::BrowsingContext::CanAccess(mozilla::dom::BrowsingContext*, bool) [docshell/base/BrowsingContext.cpp:1355]
[task 2023-01-04T19:57:08.871Z] 19:57:08     INFO - GECKO(764) | #02: mozilla::dom::BrowsingContext::FindWithNameInSubtree(nsTSubstring<char16_t> const&, mozilla::dom::BrowsingContext&) [docshell/base/BrowsingContext.cpp:1325]
[task 2023-01-04T19:57:08.871Z] 19:57:08     INFO - GECKO(764) | #03: mozilla::dom::BrowsingContext::FindWithName(nsTSubstring<char16_t> const&, bool) [docshell/base/BrowsingContext.cpp:1259]
[task 2023-01-04T19:57:08.872Z] 19:57:08     INFO - GECKO(764) | #04: nsGlobalWindowOuter::WindowExists(nsTSubstring<char16_t> const&, bool, bool) [dom/base/nsGlobalWindowOuter.cpp:4063]

Comment 1

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

:emilio, since you are the author of the regressor, bug 1805414, could you take a look?

For more information, please visit auto_nag documentation.

Comment 2

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

Set release status flags based on info from the regressing bug 1805414

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Ah, this is similar but not the same as bug 1808628... Nika, it seems this test when run on verify ends up with a broken browsing context set-up in some cases. Do you know what might be going on?

In any case pretty sure this is not really a regression, my patch just touched this test, but the window.open() is the very first thing that my patch does.

Comment 4

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

I ran this locally and caught the assertion under rr - I'd post a pernosco trace, but it's taking a while to process, so I just debugged it locally.

The error is happening because this is a Type::Content BrowsingContext, while aTarget is a Type::Chrome BrowsingContext. You cannot access a chrome BC from a content one, so they have different groups, and this assertion ends up firing.

It seems like the window for the mochitest-chrome is a content window with a system principal, and that the code is calling window.open from the toplevel (chrome) browsing context to create it. This is a cross-BCG call of window.open because the target window being called on is in the system BCG, whereas the caller is in a content BCG.

nsGlobalWindowOuter::OpenInternal then calls WindowExists which will attempt to look up an existing window by name. This call ends up passing true for aUseEntryGlobalForAccessCheck, so we access the entry global and use that instead of this for access checks. This entry global is in a different BCG, so the checks we do later end up failing. This is only possible when window.open is called from a content scope on a chrome document when we also already have a pop-up chrome window with the given name, as the assertion would pass otherwise. This explains why it only happens in verify, because otherwise the window would not exist and we'd bypass this check.

It's a bit of an awkward situation, as it's not entirely clear what the correct behaviour is when the entry global is in a different BCG than the target BrowsingContext. This can only happen for system JS, as the BCG is the scope of code execution for all non-privileged JS in our codebase, so situations like this test are the only things which end up being really relevant. The call was specifically moved to be on the toplevel BC as part of bug 1553804, in order to avoid having chrome documents with content owners, and it appears we didn't decouple the globals for this check in that case.

The easiest fix here is probably to close the window when the test is done to make sure that the state is cleaned up before the next run, though there is definitely some other weirdness there around the context-menu window. We'll probably need to do this even if we fix the assertions, as the loading code won't work if it actually ends up targeting an existing window, as it is depending on the initial about:blank document's behaviour.

Comment 5

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

Attachment: Bug 1808630 - Part 1: Clean up after test_edit_contextmenu.html, r=smaug!

This fixes the main issue, and makes the test pass by avoiding issues with the
window sticking around after test completion, messing up future runs.

Comment 6

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

Attachment: Bug 1808630 - Part 3: Handle cross-group system openers in BrowsingContext::FindWithName, r=smaug!

This is possible in test situations, as a content document with a system
principal can call methods like window.open on a chrome document, which has a
different BCG. Treat these cross-group calls like they're coming from the
target document.

Depends on D166119

Comment 7

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

Attachment: Bug 1808630 - Part 2: Rename CallerInnerWindow to IncumbentInnerWindow and add EntryInnerWindow, r=smaug!

This makes the correlation between the method names and what they do
more clear, and adds an Entry variant for the entry global.

Depends on D166119

Comment 8

[Intermittent Failures Robot]

2 failures in 2607 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 2

Platform and build breakdown:

• windows10-32-2004-qr: 2
  • debug: 2

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1808630&startday=2023-01-02&endday=2023-01-08&tree=all

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5358a3e40405
Part 1: Clean up after test_edit_contextmenu.html, r=smaug
https://hg.mozilla.org/integration/autoland/rev/c4ffc502e227
Part 2: Rename CallerInnerWindow to IncumbentInnerWindow and add EntryInnerWindow, r=smaug
https://hg.mozilla.org/integration/autoland/rev/df8c8a7158bb
Part 3: Handle cross-group system openers in BrowsingContext::FindWithName, r=smaug

Comment 10

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/5358a3e40405
https://hg.mozilla.org/mozilla-central/rev/c4ffc502e227
https://hg.mozilla.org/mozilla-central/rev/df8c8a7158bb