WindowHidden reads mActiveWindow from a content process
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox76 | --- | fixed |
People
(Reporter: hsivonen, Assigned: hsivonen)
References
Details
Attachments
(1 file)
https://searchfox.org/mozilla-central/rev/c79c0d65a183d9d38676855f455a5c6a7f7dadd3/dom/base/nsFocusManager.cpp#1008 should not read mActiveWindow
from a content process.
Assignee | ||
Comment 1•4 years ago
|
||
Assignee | ||
Comment 2•4 years ago
|
||
Assignee | ||
Comment 3•4 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #1)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3437c546e3ddeed1fe58e2cf733a63b432f6a67
Everything went orange showing that this is indeed used in content processes. Let's try doing this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14a1cb8340b494bcd48337508122dfe8683f46e3
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 4•4 years ago
|
||
Sigh. That patch makes thing very orange, too.
Assignee | ||
Comment 5•4 years ago
|
||
Looks like I forgot a line of code.
Assignee | ||
Comment 6•4 years ago
|
||
Assignee | ||
Comment 7•4 years ago
|
||
Assignee | ||
Comment 8•4 years ago
|
||
dom/events/test/test_focus_disabled.html times out on try but works for me locally.
In the local successful run, the non-parent-process code path added in the patch runs twice, and on both occasions, the case that gets executed is
WindowLowered(activeWindow);
i.e. the in-process case of window
being in the active tab.
Assignee | ||
Comment 9•4 years ago
|
||
Let's see if the problem is actually caused by another test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1aae8c2006f2f46ff91f02dec37917ea7e7b394
Comment 10•4 years ago
|
||
Tracking for Fission Nightly (M6) since this bug transitively blocks M6 bug 1614268.
Assignee | ||
Comment 11•4 years ago
|
||
Let's try not running the tests before the failing one and see if it stops failing that way:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8729266e9fa250a9aaf9ab28a07cafaf61928ed6
Assignee | ||
Comment 12•4 years ago
|
||
Looks like infra failed. Another dice roll:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb8ecb25cd97ccacfe04e47885c81372264a1806
Assignee | ||
Comment 13•4 years ago
|
||
OK. It's state left over from the previous tests.
Assignee | ||
Comment 14•4 years ago
|
||
Without the first half of previous tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=abc593940dec4e437f4e15db6f8fc61a2d40ad8a
Assignee | ||
Comment 15•4 years ago
|
||
Without the second half of previous tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a75d5325c71bfba4c1bb2c2c92654b52fda1af22
Assignee | ||
Comment 16•4 years ago
|
||
It's in the second half.
Assignee | ||
Comment 17•4 years ago
|
||
Disable from test_bug656954.html to test_bug1013412.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d6e7a78318c3dc57206d81ba33cd9fbe189091
Assignee | ||
Comment 18•4 years ago
|
||
Disable from test_bug1017086_disable.html to test_eventTimeStamp.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=978e5e4571fc954afa3842c44c4723f76f790c89
Assignee | ||
Comment 19•4 years ago
|
||
test_eventTimeStamp.html is the most likely cause but also the worst case for bisection, so let's test for disabling only it in parallel to the bisection:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bacf0769e0c758dbee09c95cdb52694bcba049c
Assignee | ||
Comment 20•4 years ago
|
||
Disable test_bug656954.html to test_bug704423.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cf62d6fb33b7fb8cb4e7f79479bd0708c5c1d83
Assignee | ||
Comment 21•4 years ago
|
||
Disable test_bug741666.html to test_bug1013412.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37acdd2adf112d70c15f6ebcc453333cf107a38c
Assignee | ||
Comment 22•4 years ago
|
||
Accidentally took the wrong branch above.
Disable test_bug1017086_disable.html to test_dblclick_explicit_original_target.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc6bbfe9c6b8e045d24c0ba7c6305fb6cda2233e
Assignee | ||
Comment 23•4 years ago
|
||
Disable test_dom_activate_event.html to test_event_screenXY_in_cross_origin_iframe.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b21259d07dc65a87db7f35e4461201afc57ba6f1
Assignee | ||
Comment 24•4 years ago
|
||
Disable test_dom_activate_event.html to test_draggableprop.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6b794d16a29e22e90b565ff7b5d7b42fb2c4bb4
Assignee | ||
Comment 25•4 years ago
|
||
Disable test_dragstart.html to test_event_screenXY_in_cross_origin_iframe.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8090ea670886527e0c6a8eeeb99f43792126b354
Assignee | ||
Comment 26•4 years ago
|
||
Disable test_dragstart.html to test_event_handler_cc.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff5f0161963dc72626f3b311627be553745aae95
Assignee | ||
Comment 27•4 years ago
|
||
Disable test_eventctors.html to test_event_screenXY_in_cross_origin_iframe.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=970c41d11ec89c2b2a71923f4639041426e6649b
Assignee | ||
Comment 28•4 years ago
|
||
Disable test_eventctors.html
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba08a5714eb571e701e66d6d01cd4e18ca151400
Assignee | ||
Comment 29•4 years ago
|
||
Disable test_eventctors_sensors.html.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=761b5db88736d9d32c81ca428a413af682f91fe4
Assignee | ||
Comment 30•4 years ago
|
||
Disable test_disabled_events.html.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd197314b9ecd817455646218eb925af04b3dea9
Assignee | ||
Comment 31•4 years ago
|
||
Disable test_event_screenXY_in_cross_origin_iframe.html.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29c252d3b692b6c046da6c5c1a99d67510f60315
Assignee | ||
Comment 32•4 years ago
|
||
The problem is dom/events/test/test_event_screenXY_in_cross_origin_iframe.html . When run alone, the test passes for me locally.
Assignee | ||
Comment 33•4 years ago
|
||
When run alone, test_event_screenXY_in_cross_origin_iframe.html runs the content-process WindowHidden
path twice, and on both occasions, it runs the in-process WindowLowered
case:
WindowLowered(activeWindow);
Assignee | ||
Comment 34•4 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #8)
dom/events/test/test_focus_disabled.html times out on try but works for me locally.
In the local successful run, the non-parent-process code path added in the patch runs twice, and on both occasions, the case that gets executed is
WindowLowered(activeWindow);
i.e. the in-process case of
window
being in the active tab.
(In reply to Henri Sivonen (:hsivonen) from comment #33)
When run alone, test_event_screenXY_in_cross_origin_iframe.html runs the content-process
WindowHidden
path twice, and on both occasions, it runs the in-processWindowLowered
case:WindowLowered(activeWindow);
When the two tests are run together, the second one times out, and the content-process WindowHidden
path runs twice. The first execution is as above, but the second one takes the
contentChild->SendWindowLowered(active);
branch.
Assignee | ||
Comment 35•4 years ago
|
||
Moreover, the second execution happens right after printing:
TEST-START | /tests/dom/events/test/test_focus_disabled.html
When test_event_screenXY_in_cross_origin_iframe.html
is run alone, the second execution of the WindowHidden
code matches a printout line:
[Child 824, Main Thread] WARNING: IPC message discarded: actor cannot send: file /opt/Projects/gecko/ipc/glue/ProtocolUtils.cpp, line 477
which happens after:
SimpleTest FINISHED
So, either way, it doesn't happen during test_event_screenXY_in_cross_origin_iframe.html
, but whether in runs the in-process case or the out-of-process case depends on whether the harness shuts down or there's a subsequent test.
Assignee | ||
Comment 36•4 years ago
|
||
On both occasions, the first execution happens before:
SimpleTest START
Assignee | ||
Comment 37•4 years ago
|
||
In both cases, the second execution comes from BrowserChild::RecvDestroy
.
Assignee | ||
Comment 38•4 years ago
|
||
The reasons why we get there are very different. In the single-test case, the parent runs nsGlobalWindowOuter::ReallyCloseWindow
and the destruction propagates from there.
In the two-test case, another content process destroys its previous nsDocumentViewer
from nsDocumentViewer::Show
and the destruction propagates via BrowserBridge
destruction to the process that ends up running WindowHidden
.
My hypothesis is that the active BrowsingContext
has updated too soon and WindowHidden
would prefer to still see whatever the previously active BrowsingContext
was.
Assignee | ||
Comment 39•4 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #38)
In the two-test case, another content process destroys its previous
nsDocumentViewer
fromnsDocumentViewer::Show
and the destruction propagates viaBrowserBridge
destruction to the process that ends up runningWindowHidden
.
The other process where the destruction chain starts from appears to be the out-of-process iframe and WindowHidden
appears to run in top-level content.
(In reply to Henri Sivonen (:hsivonen) from comment #38)
My hypothesis is that the active
BrowsingContext
has updated too soon andWindowHidden
would prefer to still see whatever the previously activeBrowsingContext
was.
The BrowsingContext
arrived to the process running WindowHidden
as the result of a BrowsingContext
switching the process. So far, I suspect that the BrowsingContext
hosting the top-level Web content is the one that changed processes. I'm not quite sure, yet, but the problem could be that the top-level BrowsingContext
switches processes between the two tests, and the new process receives WindowHidden
that's logically meant for the old torn-down process.
Assignee | ||
Comment 40•4 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #39)
The other process where the destruction chain starts from appears to be the out-of-process iframe
Hmm. Maybe I'm confused, and the destruction chain started from the old top-level content.
Assignee | ||
Comment 41•4 years ago
|
||
I'm also confused about why there'd be a process switch between the top-level test files. They are supposed to be from the same origin.
Assignee | ||
Comment 42•4 years ago
|
||
I probably got the processes reversed: The top-level content doesn't migrate between processes. The oop-iframe-does (from about:blank?). And the oop iframe runs the second WindowHidden
. So why does the second WindowHidden
run the in-process case when the test is standalone?
Assignee | ||
Comment 43•4 years ago
|
||
It appears that the code being changed here doesn't run at all in the non-Fission case, so the non-Fission case doesn't give an idea of what should happen. Instead, I assume that this is all about tearing down the out-of-process iframe. Perhaps we don't even need to run the normal steps in that case.
Assignee | ||
Comment 44•4 years ago
•
|
||
Let's see what breaks if the out-of-process iframe case doesn't do anything:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d660f34d9918297d0506e592643f6e9f3417923
Assignee | ||
Comment 45•4 years ago
|
||
Don't compare with Top()
:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9ebf0aee132dd69ba2bc45482f18b07fb87450e
Comment 46•4 years ago
|
||
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a96b3ee660d1 Avoid reading mActiveWindow in a content process in WindowHidden. r=NeilDeakin
Comment 47•4 years ago
|
||
bugherder |
Description
•