Closed Bug 1620173 Opened 5 years ago Closed 5 years ago

WindowHidden reads mActiveWindow from a content process

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla76
Project Flags:
Fission Milestone M6
Tracking Flags:
Tracking Status
firefox76 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file)

Bug 1620173 - Avoid reading mActiveWindow in a content process in WindowHidden.
47 bytes, text/x-phabricator-request
Details | Review
Blocks: 1618135

(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: nobody → hsivonen
Status: NEW → ASSIGNED

Sigh. That patch makes thing very orange, too.

Looks like I forgot a line of code.

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.

Let's see if the problem is actually caused by another test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1aae8c2006f2f46ff91f02dec37917ea7e7b394

Tracking for Fission Nightly (M6) since this bug transitively blocks M6 bug 1614268.

Fission Milestone: --- → M6

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

OK. It's state left over from the previous tests.

It's in the second half.

Disable from test_bug656954.html to test_bug1013412.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d6e7a78318c3dc57206d81ba33cd9fbe189091

Disable from test_bug1017086_disable.html to test_eventTimeStamp.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=978e5e4571fc954afa3842c44c4723f76f790c89

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

Disable test_bug656954.html to test_bug704423.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cf62d6fb33b7fb8cb4e7f79479bd0708c5c1d83

Disable test_bug741666.html to test_bug1013412.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37acdd2adf112d70c15f6ebcc453333cf107a38c

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

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

Disable test_dom_activate_event.html to test_draggableprop.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6b794d16a29e22e90b565ff7b5d7b42fb2c4bb4

Disable test_dragstart.html to test_event_screenXY_in_cross_origin_iframe.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8090ea670886527e0c6a8eeeb99f43792126b354

Disable test_dragstart.html to test_event_handler_cc.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff5f0161963dc72626f3b311627be553745aae95

Disable test_eventctors.html to test_event_screenXY_in_cross_origin_iframe.html, inclusive.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=970c41d11ec89c2b2a71923f4639041426e6649b

The problem is dom/events/test/test_event_screenXY_in_cross_origin_iframe.html . When run alone, the test passes for me locally.

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);

(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-process WindowLowered 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.

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.

On both occasions, the first execution happens before:

SimpleTest START

In both cases, the second execution comes from BrowserChild::RecvDestroy.

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.

(In reply to Henri Sivonen (:hsivonen) from comment #38)

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.

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 and WindowHidden would prefer to still see whatever the previously active BrowsingContext 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.

(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.

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.

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?

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.

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

Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a96b3ee660d1 Avoid reading mActiveWindow in a content process in WindowHidden. r=NeilDeakin

https://hg.mozilla.org/mozilla-central/rev/a96b3ee660d1

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox76: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla76
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: