Closed Bug 1594752 Opened 5 years ago Closed 5 years ago

[Fission] Switch to fission-compatible JSWindowActor for DOMTitleChanged

Categories

(Toolkit :: UI Widgets, task, P3)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla78
Project Flags:
Fission Milestone M6
Tracking Flags:
Tracking Status
firefox72 --- wontfix
firefox78 --- fixed

People

(Reporter: Gijs, Assigned: enndeakin)

References

Details

Attachments

(2 files)

Bug 1594752, expose WindowGlobalParent's document title attribute to script, and fire a pagetitlechanged event on the frame/browser when it changes, r=nika
47 bytes, text/x-phabricator-request
Details | Review
Bug 1594752, use WindowGlobalParent's documentTitle to update tab titles rather than sending messages and events between processes, r=gijs
47 bytes, text/x-phabricator-request
Details | Review

Brian pointed out in https://phabricator.services.mozilla.com/D52022#inline-316024 that we're listening for DOMTitleChanged in the parent via an event listener in tabbrowser, and via a message manager + frame script infrastructure in the child, so there's logic that's duplicated across those and the tabbrowser has to have 2 types of listeners, which isn't ideal.

We should unify this to use a simple JSWindowActor, so that we only care about the toplevel frame (we never do anything with subframe titles, AFAICT) and perhaps we could even get away with not having a child actor at all - the DOM side of things could just get the actor, invoke sendAsyncMessage, and be done with it.

The parent side of things would be in toolkit, would update browser.contentTitle, and emit an event on the browser. tabbrowser.js could then listen for this event.

This is a little bit cumbersome (message-implies-event-implies browser UI change, vs. just having the browser wait for the event) but probably more correct than having the actor only in browser/ code, in which case it's not clear how we think this should work in other toolkit situations (which I guess would include geckoview).

While we're here, we should fix handling of avoiding showing URLs for about: pages to apply to all about: pages, not just parent-process system principal ones - while making sure not to include about:blank in that change

See Also: → 1588142

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Fission Milestone: --- → ?
Fission Milestone: ? → M5

Blocks Fission dogfooding (M5).

Priority: -- → P3

So, Gijs, what would be the original event to listen to? DOMTitleChanged in the parent?

Assignee: nobody → dteller
Flags: needinfo?(gijskruitbosch+bugs)

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)

So, Gijs, what would be the original event to listen to? DOMTitleChanged in the parent?

Not sure I understand the question. I think the answer is having a separate event in the parent, fired on the browser, that consumers would listen to.

Elaborating, I think you'd want to define an actor at https://searchfox.org/mozilla-central/source/toolkit/modules/ActorManagerParent.jsm#33, define only a parent module, not define allFrames, and you'd update https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/dom/base/Document.cpp#8402 to call sendAsyncMessage only if the document is the toplevel browsingcontext.

The parent actor would fire a BrowserTitleChanged event on the browser element, and the things that currently use the frame message manager to listen for DOMTitleChanged messages would look for that event instead.

Dispatching the async message directly from C++ looks like it might interact in "interesting" ways with android, in which case a less elegant but easier-to-convert approach would be to also have a child actor (that still forwards the event to the parent via the actor's messaging system) and to keep the DOM code the same.

Does that clarify?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dteller)

Elaborating, I think you'd want to define an actor at https://searchfox.org/mozilla-central/source/toolkit/modules/ActorManagerParent.jsm#33, define only a parent module, not define allFrames, and you'd update https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/dom/base/Document.cpp#8402 to call sendAsyncMessage only if the document is the toplevel browsingcontext.

So this sendAsyncMessage would replace the nsContentUtils::DispatchChromeEvent with event DOMTitleChanged, right?

Flags: needinfo?(dteller) → needinfo?(gijskruitbosch+bugs)

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #6)

Elaborating, I think you'd want to define an actor at https://searchfox.org/mozilla-central/source/toolkit/modules/ActorManagerParent.jsm#33, define only a parent module, not define allFrames, and you'd update https://searchfox.org/mozilla-central/rev/3811b11b5773c1dccfe8228bfc7143b10a9a2a99/dom/base/Document.cpp#8402 to call sendAsyncMessage only if the document is the toplevel browsingcontext.

So this sendAsyncMessage would replace the nsContentUtils::DispatchChromeEvent with event DOMTitleChanged, right?

Yes, though I don't know how that interacts with android, per my other para:

Dispatching the async message directly from C++ looks like it might interact in "interesting" ways with android, in which case a less elegant but easier-to-convert approach would be to also have a [jsm] child actor (that still forwards the event to the parent via the actor's messaging system) and to keep the DOM code the same.

Flags: needinfo?(gijskruitbosch+bugs)

Deferring to Fission Nightly (M6)

Unassigning David because he is focusing on higher priority work at the moment.

Nika says window title sync'd by WindowContext. Is this bug still relevant?

Assignee: dteller → nobody
Fission Milestone: M5 → M6

(In reply to Chris Peterson [:cpeterson] from comment #8)

Deferring to Fission Nightly (M6)

Unassigning David because he is focusing on higher priority work at the moment.

Nika says window title sync'd by WindowContext. Is this bug still relevant?

I don't see anything in https://searchfox.org/mozilla-central/source/docshell/base/WindowContext.h that looks like a window title; we'd also need some kind of event to notify the parent that the title has changed. Nika, can you clarify what you meant when you said this?

Flags: needinfo?(nika)

(In reply to :Gijs (he/him) from comment #9)

I don't see anything in https://searchfox.org/mozilla-central/source/docshell/base/WindowContext.h that looks like a window title; we'd also need some kind of event to notify the parent that the title has changed. Nika, can you clarify what you meant when you said this?

Sorry, I was referring to WindowGlobalParent, rather than WindowContext. https://searchfox.org/mozilla-central/rev/49ed791eec93335abfe6c2880f84c324e73e47e6/dom/ipc/WindowGlobalParent.h#103. You'd need to fire the event/observer notification around here: https://searchfox.org/mozilla-central/rev/49ed791eec93335abfe6c2880f84c324e73e47e6/dom/ipc/WindowGlobalParent.cpp#252

Flags: needinfo?(nika)
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED

Fix up the browser_tab_label_during_restore.js test to wait for the right number of tab title changes, since the timing of the tab title updating has now changed.

Depends on D72561

Attachment #9143480 - Attachment description: Bug 1594752, use WindowGlobalParent's documentTitle to update tab titles rather than sending messages and events between processes, r=mconley → Bug 1594752, use WindowGlobalParent's documentTitle to update tab titles rather than sending messages and events between processes, r=gijs
Pushed by neil@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/801d1e226947 expose WindowGlobalParent's document title attribute to script, and fire a pagetitlechanged event on the frame/browser when it changes, r=nika https://hg.mozilla.org/integration/autoland/rev/b54107a687e4 use WindowGlobalParent's documentTitle to update tab titles rather than sending messages and events between processes, r=Gijs

Please have a look over these failures too: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64%2Fdebug-mochitest-browser-chrome-fis-e10s-15%2Cm-fis%28bc15%29&tochange=515f4054b1ccaa49b1f25f70f4e3a6f610184690&fromchange=b54107a687e496003a3c155a5efc2d0b6b91a6d2&selectedTaskRun=GGWACX8wQY6vEMPVNPvKgA-0

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=301752697&repo=autoland

[task 2020-05-11T17:28:24.224Z] 17:28:24     INFO - TEST-START | browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js
[task 2020-05-11T17:28:24.325Z] 17:28:24     INFO - GECKO(1227) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpDKrtvH.mozrunner/runtests_leaks_tab_pid6673.log
[task 2020-05-11T17:28:24.326Z] 17:28:24     INFO - GECKO(1227) | [6673, Main Thread] WARNING: XPCOM_MEM_BLOAT_LOG is set, disabling native allocations.: file /builds/worker/checkouts/gecko/tools/profiler/core/platform.cpp, line 225
[task 2020-05-11T17:28:24.695Z] 17:28:24     INFO - GECKO(1227) | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2020-05-11T17:28:24.695Z] 17:28:24     INFO - GECKO(1227) | [Child 6673, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /builds/worker/checkouts/gecko/dom/media/CubebUtils.cpp, line 363
[task 2020-05-11T17:28:24.776Z] 17:28:24     INFO - GECKO(1227) | [Child 6673, Main Thread] WARNING: could not set real-time limit at process startup: file /builds/worker/checkouts/gecko/dom/ipc/ContentChild.cpp, line 1640
[task 2020-05-11T17:28:24.792Z] 17:28:24     INFO - GECKO(1227) | [Child 6673: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x7f51f87c5c00 == 1 [pid = 6673] [id = {32baa9de-61e7-4f83-8026-927bbf0c3c43}]
[task 2020-05-11T17:28:24.814Z] 17:28:24     INFO - GECKO(1227) | [Child 6673: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 1 (0x7f520e093350) [pid = 6673] [serial = 1] [outer = (nil)]
[task 2020-05-11T17:28:24.817Z] 17:28:24     INFO - GECKO(1227) | [Child 6673: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 2 (0x7f51f296c400) [pid = 6673] [serial = 2] [outer = 0x7f520e093350]
[task 2020-05-11T17:28:25.146Z] 17:28:25     INFO - GECKO(1227) | [Child 6673: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 3 (0x7f51f329cc00) [pid = 6673] [serial = 3] [outer = 0x7f520e093350]
[task 2020-05-11T17:28:25.512Z] 17:28:25     INFO - GECKO(1227) | [Parent 1227, Main Thread] WARNING: Need BrowserChild to get the nativeWindow from!: file /builds/worker/checkouts/gecko/widget/PuppetWidget.cpp, line 1086
[task 2020-05-11T17:28:25.528Z] 17:28:25     INFO - GECKO(1227) | [Child 1317: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x7fb421d7a000 == 6 [pid = 1317] [id = {a47ab77f-92c6-4d45-9402-c73aca580e04}]
[task 2020-05-11T17:28:25.529Z] 17:28:25     INFO - GECKO(1227) | [Child 1317: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 18 (0x7fb41d59f7d0) [pid = 1317] [serial = 1789] [outer = (nil)]
[task 2020-05-11T17:28:25.529Z] 17:28:25     INFO - GECKO(1227) | [Child 1317: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 19 (0x7fb421d7cc00) [pid = 1317] [serial = 1790] [outer = 0x7fb41d59f7d0]
[task 2020-05-11T17:28:25.544Z] 17:28:25     INFO - GECKO(1227) | [Child 1317, Main Thread] WARNING: Fallback to BasicLayerManager: file /builds/worker/checkouts/gecko/dom/ipc/BrowserChild.cpp, line 2663
[task 2020-05-11T17:28:25.632Z] 17:28:25     INFO - GECKO(1227) | [Child 1317: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 20 (0x7fb42c56dc00) [pid = 1317] [serial = 1791] [outer = 0x7fb41d59f7d0]
[task 2020-05-11T17:28:25.970Z] 17:28:25     INFO - GECKO(1227) | [Child 1301: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x7f7167608000 == 2 [pid = 1301] [id = {1aad28d1-2b47-4be2-9d69-e9eac9e2f2c8}]
[task 2020-05-11T17:28:25.971Z] 17:28:25     INFO - GECKO(1227) | [Child 1301: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 4 (0x7f7165d04b70) [pid = 1301] [serial = 270] [outer = (nil)]
[task 2020-05-11T17:28:25.972Z] 17:28:25     INFO - GECKO(1227) | [Child 1301: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 5 (0x7f716760a800) [pid = 1301] [serial = 271] [outer = 0x7f7165d04b70]
[task 2020-05-11T17:28:26.049Z] 17:28:26     INFO - GECKO(1227) | [Child 6673: Main Thread]: I/DocShellAndDOMWindowLeak ++DOCSHELL 0x7f51f2975000 == 2 [pid = 6673] [id = {1aad28d1-2b47-4be2-9d69-e9eac9e2f2c8}]
[task 2020-05-11T17:28:26.051Z] 17:28:26     INFO - GECKO(1227) | [Child 6673: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 4 (0x7f51f88d9bc0) [pid = 6673] [serial = 4] [outer = (nil)]
[task 2020-05-11T17:28:26.051Z] 17:28:26     INFO - GECKO(1227) | [Child 6673: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 5 (0x7f51f2976400) [pid = 6673] [serial = 5] [outer = 0x7f51f88d9bc0]
[task 2020-05-11T17:28:26.052Z] 17:28:26     INFO - GECKO(1227) | [Child 1301: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 6 (0x7f71676e7800) [pid = 1301] [serial = 272] [outer = 0x7f7165d04b70]
[task 2020-05-11T17:28:26.290Z] 17:28:26     INFO - TEST-INFO | started process screentopng
[task 2020-05-11T17:28:26.590Z] 17:28:26     INFO - TEST-INFO | screentopng: exit 0
[task 2020-05-11T17:28:26.591Z] 17:28:26     INFO - Buffered messages logged at 17:28:24
[task 2020-05-11T17:28:26.591Z] 17:28:26     INFO - Entering test bound testDuplicateTab
[task 2020-05-11T17:28:26.592Z] 17:28:26     INFO - Buffered messages logged at 17:28:25
[task 2020-05-11T17:28:26.593Z] 17:28:26     INFO - Extension loaded
[task 2020-05-11T17:28:26.596Z] 17:28:26     INFO - Console message: Warning: attempting to write 25863 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes.
[task 2020-05-11T17:28:26.597Z] 17:28:26     INFO - Buffered messages finished
[task 2020-05-11T17:28:26.597Z] 17:28:26     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js | uncaught exception - TypeError: can't access property "documentTitle", this.browsingContext.currentWindowGlobal is null at get contentTitle@chrome://global/content/elements/browser-custom-element.js:730:7
[task 2020-05-11T17:28:26.597Z] 17:28:26     INFO - setTabTitle@chrome://browser/content/tabbrowser.js:1423:19
[task 2020-05-11T17:28:26.598Z] 17:28:26     INFO - onLocationChange@chrome://browser/content/tabbrowser.js:6117:22
[task 2020-05-11T17:28:26.598Z] 17:28:26     INFO - _callProgressListeners@resource://gre/modules/RemoteWebProgress.jsm:75:31
[task 2020-05-11T17:28:26.599Z] 17:28:26     INFO - onLocationChange@resource://gre/modules/RemoteWebProgress.jsm:119:10
[task 2020-05-11T17:28:26.599Z] 17:28:26     INFO - 
[task 2020-05-11T17:28:26.599Z] 17:28:26     INFO - Stack trace:
[task 2020-05-11T17:28:26.600Z] 17:28:26     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1950
[task 2020-05-11T17:28:26.600Z] 17:28:26     INFO - resource://gre/modules/RemoteWebProgress.jsm:_callProgressListeners:75
[task 2020-05-11T17:28:26.601Z] 17:28:26     INFO - resource://gre/modules/RemoteWebProgress.jsm:onLocationChange:119
[task 2020-05-11T17:28:26.601Z] 17:28:26     INFO - GECKO(1227) | JavaScript error: chrome://global/content/elements/browser-custom-element.js, line 730: TypeError: can't access property "documentTitle", this.browsingContext.currentWindowGlobal is null
[task 2020-05-11T17:28:26.601Z] 17:28:26     INFO - GECKO(1227) | JavaScript error: chrome://global/content/elements/browser-custom-element.js, line 730: TypeError: can't access property "documentTitle", this.browsingContext.currentWindowGlobal is null
[task 2020-05-11T17:28:26.602Z] 17:28:26     INFO - GECKO(1227) | [Child 1301: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 5 (0x7f716760b000) [pid = 1301] [serial = 266] [outer = (nil)] [url = about:blank]
[task 2020-05-11T17:28:26.602Z] 17:28:26     INFO - GECKO(1227) | [Child 1301: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 0x7f716760cc00 == 1 [pid = 1301] [id = {cb83a5d6-ff00-4a49-9c1a-35f6b38f5d97}] [url = about:blank]
[task 2020-05-11T17:28:26.603Z] 17:28:26     INFO - GECKO(1227) | [Child 1301: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 0x7f7167608000 == 0 [pid = 1301] [id = {1aad28d1-2b47-4be2-9d69-e9eac9e2f2c8}] [url = http://example.net/]
[task 2020-05-11T17:28:26.603Z] 17:28:26     INFO - Not taking screenshot here: see the one that was previously logged
Pushed by neil@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e781cf38f088 expose WindowGlobalParent's document title attribute to script, and fire a pagetitlechanged event on the frame/browser when it changes, r=nika https://hg.mozilla.org/integration/autoland/rev/66cc44b67170 use WindowGlobalParent's documentTitle to update tab titles rather than sending messages and events between processes, r=Gijs

https://hg.mozilla.org/mozilla-central/rev/e781cf38f088
https://hg.mozilla.org/mozilla-central/rev/66cc44b67170

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox78: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
No longer regressions: 1638148
Regressions: 1638148

Looks like browser.browingContext is null for the crash page that is loaded, when the title is retrieved. The current code just returns an empty string during that point in the test. I'll just add a null-check.

Flags: needinfo?(enndeakin)
Pushed by neil@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e7e729d979d7 expose WindowGlobalParent's document title attribute to script, and fire a pagetitlechanged event on the frame/browser when it changes, r=nika https://hg.mozilla.org/integration/autoland/rev/b3c11aab3e29 use WindowGlobalParent's documentTitle to update tab titles rather than sending messages and events between processes, r=Gijs

https://hg.mozilla.org/mozilla-central/rev/e7e729d979d7
https://hg.mozilla.org/mozilla-central/rev/b3c11aab3e29

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

Attachment

General

Creator:
Created:
Updated:
Size: