1646498 - Fix GetInProcessTop usage in nsAutoSyncOperation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Kris Maglione [:kmag]]

It's possible that we only need to in-process windows when we're blocking for a sync operation, but we definitely need to correctly handle the case where the top window is not in the same process as the window that enters the sync operation, and when there are OOP interstitials with in-process descendants.

Comment 1

[Chris Peterson [:cpeterson]]

assign to kmag for M6c

Comment 2

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

As kmag is expecting here, we probably don't need to do any cross-process work here, as we're only working with the in-process documents. We can possibly get away with grabbing our BrowsingContextGroup, and recursively iterating over every BrowsingContext, setting them as in a sync operation if they're in-process.

Comment 3

[u608768]

Attachment: a1.html

Comment 4

[u608768]

Attachment: a2.html

Comment 5

[u608768]

Attachment: b.html

Comment 6

[u608768]

The document at https://bugzilla.mozilla.org/attachment.cgi?id=9168619 has two processes. The toplevel document and double-nested frame are in the same process. In Firefox, both with and without fission, opening an alert in any of the three frames suspends all intervals. Chrome only suspends intervals for the process that the alert was opened for (eg., opening the alert in either the toplevel or double-nested frame will not pause the middle frame's counter, and opening the alert in the middle frame will not pause the interval in the other two frames).

Comment 7

[u608768]

I'm still seeing comment 6 with the changes from comment 2. It appears someone else is suspending the timeout:

• The toplevel browser element sends a "EnterModalState" to all process roots here.

• The middle frame receives that message and calls nsDOMWindowUtils::EnterModalState(), and we eventually suspend the timeout in that process:

  #0  mozilla::dom::TimeoutManager::Suspend() at /home/kashav/src/hg.mozilla.org/mozilla-central/dom/base/TimeoutManager.cpp:1145
  #1  nsGlobalWindowInner::Suspend() at /home/kashav/src/hg.mozilla.org/mozilla-central/dom/base/nsGlobalWindowInner.cpp:5272
  #2  nsGlobalWindowOuter::EnterModalState() at /home/kashav/src/hg.mozilla.org/mozilla-central/dom/base/nsGlobalWindowOuter.cpp:6280
  #3  nsDOMWindowUtils::EnterModalState() at /home/kashav/src/hg.mozilla.org/mozilla-central/dom/base/nsDOMWindowUtils.cpp:2111
  ...
  

I should probably find something that will actually be fixed by making nsAutoSyncOperation work with fission...

Comment 8

[u608768]

(In reply to :kashav from comment #7)

I should probably find something that will actually be fixed by making nsAutoSyncOperation work with fission...

I was able to get this to work using MutationObservers. While in an nsAutoSyncOperation, we block mutation notifications for toplevel documents, but don't block them for in-process descendants which have out-of-process ancestors.

Comment 9

[u608768]

Attachment: Bug 1646498 - Fix GetInProcessTop usage in nsAutoSyncOperation, r?kmag

This makes it so we that correctly set nsIGlobalObject::IsInSyncOperation for
in-process descendant frames that have out-of-process ancestors.

Comment 10

[u608768]

Attachment: Bug 1646498 - Update callers of nsAutoSyncOperation to use AutoMarkDocumentTreeInSync, r?kmag

Depends on D86860

Comment 11

[u608768]

Comment on attachment 9168622 [details]
b.html

Will file a followup for the timeout compat bugs.

Comment 12

[Neha Kochar [:neha]]

Steven said he can finish up Kashav's patches - thanks Steven!

Comment 13

[u608768]

Attachment: Bug 1646498 - Fix GetInProcessTop usage in nsAutoSyncOperation, r?kmag

This makes it so that we correctly set nsIGlobalObject::IsInSyncOperation
for in-process descendant frames that have out-of-process ancestors.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a911e7cbb478
Fix GetInProcessTop usage in nsAutoSyncOperation, r=kmag

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4768398204a
Backed out changeset a911e7cbb478 for open-features-negative-innerwidth-innerheight.html failures CLOSED TREE

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset a911e7cbb478 (bug 1646498) for open-features-negative-innerwidth-innerheight.html failures.

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=f6_9O6IfRL-GfEUgUIQ3vg.0&fromchange=792443abbd75d869593eb9d915a564f8ccf9f24d&searchStr=windows%2C10%2Cx64%2Copt%2Cweb%2Cplatform%2Ctests%2Ctest-windows10-64%2Fopt-web-platform-tests-e10s%2Cwpt3&tochange=b4768398204a406b9847f6a83f6404f1e03827fb

Backout link: https://hg.mozilla.org/integration/autoland/rev/b4768398204a406b9847f6a83f6404f1e03827fb

Failure log: https://treeherder.mozilla.org/logviewer?job_id=327860748&repo=autoland&lineNumber=4263

...
[task 2021-01-26T19:53:34.872Z] 19:53:34     INFO - TEST-START | /intersection-observer/multiple-targets.html
[task 2021-01-26T19:53:34.873Z] 19:53:34     INFO - Closing window 79
[task 2021-01-26T19:53:35.137Z] 19:53:35     INFO - .....
[task 2021-01-26T19:53:35.137Z] 19:53:35     INFO - TEST-OK | /intersection-observer/multiple-targets.html | took 266ms
[task 2021-01-26T19:53:35.138Z] 19:53:35     INFO - TEST-START | /intersection-observer/multiple-thresholds.html
[task 2021-01-26T19:53:35.139Z] 19:53:35     INFO - Closing window 81
[task 2021-01-26T19:53:35.543Z] 19:53:35     INFO - 
[task 2021-01-26T19:53:35.543Z] 19:53:35     INFO - TEST-UNEXPECTED-TIMEOUT | /html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-innerwidth-innerheight.html | features "innerwidth=-404" should NOT set "width=404" - Test timed out
[task 2021-01-26T19:53:35.544Z] 19:53:35     INFO - 
[task 2021-01-26T19:53:35.544Z] 19:53:35     INFO - TEST-UNEXPECTED-TIMEOUT | /html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-innerwidth-innerheight.html | features "innerwidth=-404.5" should NOT set "width=404" - Test timed out
[task 2021-01-26T19:53:35.546Z] 19:53:35     INFO - 
[task 2021-01-26T19:53:35.546Z] 19:53:35     INFO - TEST-UNEXPECTED-TIMEOUT | /html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-innerwidth-innerheight.html | features "innerwidth=-404e1" should NOT set "width=404" - Test timed out
[task 2021-01-26T19:53:35.548Z] 19:53:35     INFO - 
[task 2021-01-26T19:53:35.548Z] 19:53:35     INFO - TEST-UNEXPECTED-TIMEOUT | /html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-innerwidth-innerheight.html | features "innerheight=-404" should NOT set "height=404" - Test timed out
[task 2021-01-26T19:53:35.550Z] 19:53:35     INFO - 
[task 2021-01-26T19:53:35.550Z] 19:53:35     INFO - TEST-UNEXPECTED-TIMEOUT | /html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-innerwidth-innerheight.html | features "innerheight=-404.5" should NOT set "height=404" - Test timed out
[task 2021-01-26T19:53:35.552Z] 19:53:35     INFO - TEST-UNEXPECTED-TIMEOUT | /html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-negative-innerwidth-innerheight.html | expected OK
[task 2021-01-26T19:53:35.552Z] 19:53:35     INFO - TEST-INFO took 60551ms
[task 2021-01-26T19:53:35.555Z] 19:53:35     INFO - PID 5864 | 1611690815555	Marionette	INFO	Stopped listening on port 51296

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

Could you, please, also take a look at these intermittent crashes? Could them be caused by the backed out changes?
https://treeherder.mozilla.org/logviewer?job_id=327864078&repo=autoland&lineNumber=4725

Comment 18

[u608768]

(In reply to Bogdan Tara[:bogdan_tara | bogdant] from comment #17)

Could you, please, also take a look at these intermittent crashes? Could them be caused by the backed out changes?
https://treeherder.mozilla.org/logviewer?job_id=327864078&repo=autoland&lineNumber=4725

They were, sorry for the noise. Looking at the failures.

Comment 19

[u608768]

Failing WPTs appear to be fixed: https://treeherder.mozilla.org/jobs?repo=try&revision=8f4fa4d773cfbb9e034e877808c40c8a9a6bde91.

The intermittent crashes should be fixed too, but still have to verify.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90879d7de4f5
Fix GetInProcessTop usage in nsAutoSyncOperation, r=kmag

Comment 21

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/90879d7de4f5