Convert <browser> XBL binding to a Custom Element

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks 5 bugs)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

a year ago
Right now we have a binding tree like this https://bgrins.github.io/xbl-analysis/tree/#browser.

browser
    remote-browser
        tabbrowser-remote-browser
    tabbrowser-browser

It's possible we could reuse the same mechanism used for <tabbrowser> in Bug 1392352 to convert these to JS classes that match the inheritance pattern here, and then either flatten down to a single XBL binding which does a switch in the constructor to initialize the correct class, or keep all 4 bindings each of which initializes the correct class.

For example:

  class Browser {}
  class RemoteBrowser extends Browser {}
  class TabBrowserRemoteBrowser extends RemoteBrowser {}
  class TabBrowserBrowser extends Browser {}
(Assignee)

Comment 1

a year ago
Relevant CSS rules for attaching bindings:

browser {
  -moz-binding: url("chrome://global/content/bindings/browser.xml#browser");
}

browser[remote=true]:not(.lightweight) {
  -moz-binding: url("chrome://global/content/bindings/remote-browser.xml#remote-browser");
}

.browserStack > browser {
  -moz-binding: url("chrome://browser/content/tabbrowser.xml#tabbrowser-browser");
}

.browserStack > browser[remote="true"] {
  -moz-binding: url("chrome://browser/content/tabbrowser.xml#tabbrowser-remote-browser");
}
(Assignee)

Comment 2

a year ago
They also have this:

  <content clickthrough="never">
    <children/>
  </content>

From a quick search I don't see any places where a <browser> element has child content so the <children /> may not be necessary.  We may be able to remove the clickthrough attribute altogether by changing NodeAllowsClickThrough to return false in the case of browser/tree elements and true for a scrollbar.

Updated

a year ago
Assignee: nobody → ntim.bugs
(Assignee)

Comment 3

a year ago
LOC per binding: browser: 1711, remote-browser: 580, tabbrowser-remote-browser: 32, tabbrowser-browser: 31

The tabbrowser variants appear to only override the `loadURIWithFlags` method to call the global _loadURIWithFlags function from browser.js: https://searchfox.org/mozilla-central/search?q=_loadURIWithFlags&path=.
Another possible path is changing current usages of XUL <browser> to use <iframe mozbrowser>/<iframe> depending on the context they're being used in. Since <iframe mozbrowser> is already used in the responsive design mode, I suspect it might not be much more work than migrating to a JS class.
Not that this is a blocker for fixing the issue, but <browser> is used a few times in comm-central so we'd appreciate some time to catch up in case the migration is more than just a little search and replace.

Updated

a year ago
Depends on: 1442058
Gijs, :bgrins, Is the .lightweight class on browser still used ? https://dxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#180

I tried doing searches on SearchFox and couldn't find anything.

I'm looking if there are easy simplifications to make if that "lightweight" mode is unsupported.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 7

a year ago
(In reply to Tim Nguyen :ntim from comment #6)
> Gijs, :bgrins, Is the .lightweight class on browser still used ?
> https://dxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#180
> 
> I tried doing searches on SearchFox and couldn't find anything.
> 
> I'm looking if there are easy simplifications to make if that "lightweight"
> mode is unsupported.

It's used for reftests: https://searchfox.org/mozilla-central/source/layout/tools/reftest/reftest.jsm#185
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bgrinstead)

Updated

a year ago
Depends on: 1444760

Updated

a year ago
Depends on: 1445096

Updated

a year ago
Depends on: 1445099

Updated

a year ago
Depends on: 1445100

Updated

a year ago
Priority: -- → P5
Unassigning to reflect real status.
Assignee: ntim.bugs → nobody
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
(Assignee)

Comment 10

9 months ago
I think we have the pieces now in place to handle these elements directly with Custom Elements:

- Multiple elements can be defined per tag name as of bug 1460962. Will need to change [remote=true] to something [is=remote-browser]. 
- We can implement nsIBrowser / nsIRemoteBrowser as of Bug 1461742

It'd still be nice to not have to deal with the tabbrowser variations - which I think could be done if we came up with some way to override loadURI on the base classes in the browser window without needing to subclass.  Although we could replicate the current class structure and have more customized built-ins if neeed.
Depends on: 1461742, 1460962
See Also: → 1437638, 1478139
(Assignee)

Updated

9 months ago
Depends on: 1478372
(Assignee)

Updated

8 months ago
Depends on: 1483274
(Assignee)

Updated

8 months ago
Depends on: 1484326
(Assignee)

Updated

8 months ago
Depends on: 1486079
(Assignee)

Comment 12

8 months ago
Also use a plain object as the observer
(Assignee)

Updated

7 months ago
Depends on: 1492967
Attachment #9005030 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Summary: Investigate converting <browser> and children XBL bindings to JS classes → Convert <browser> XBL binding to a Custom Element
(Assignee)

Updated

7 months ago
Depends on: 1494103
(Assignee)

Updated

7 months ago
Attachment #9005031 - Attachment is obsolete: true
(Assignee)

Comment 14

7 months ago
The latest version up on phab is generally green in opt builds, except for a couple of tests. Now trying to figure out why it's are leaking docshells in some of the debug mochitest chunks.
(Assignee)

Updated

7 months ago
Depends on: 1496067
(Assignee)

Updated

7 months ago
No longer depends on: 1445096
See Also: → 1445096
(Assignee)

Updated

6 months ago
Depends on: 1492326
(Assignee)

Updated

4 months ago
See Also: → 1514757
(Assignee)

Comment 17

4 months ago
Steps to build:

this.observer instead of this
./mach eslint toolkit/content/widgets/browser.js --fix
(Assignee)

Comment 19

4 months ago
Down to (I think) one test failure - a leak with `./mach mochitest test_messageChannelWithMessageManager.xul`
(Assignee)

Comment 20

4 months ago
Nathan, I was talking with :mccr8 who suggested you might be able to help figure out what is going on with this leak. Have you seen something like this before? https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218384020&repo=try&lineNumber=7434

 0:13.52 INFO      |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
 0:13.52 INFO      |                                      | Per-Inst   Leaked|   Total      Rem|
 0:13.52 INFO    0 |TOTAL                                 |       45     1808|   19228       19|
 0:13.52 INFO   41 |CondVar                               |       80      160|      52        2|
 0:13.52 INFO  125 |Mutex                                 |      104      208|     300        2|
 0:13.52 INFO  215 |SchedulerEventTarget                  |       48      432|      18        9|
 0:13.52 INFO  256 |TabGroup                              |      392      392|       1        1|
 0:13.52 INFO  263 |ThrottledEventQueue                   |       40       80|       2        2|
 0:13.52 INFO  264 |ThrottledEventQueue::Inner            |      264      528|       2        2|
 0:13.52 INFO  435 |nsTArray_base                         |        8        8|    4824        1|

This is the last test failure I see when migrating <xul:browser /> away from XBL, which makes me think this may be an existing issue that's getting triggered by some kind of a timing change (since <browser> elements are used all over the place in basically every mochitest and this is the only place we see it): https://treeherder.mozilla.org/#/jobs?repo=try&revision=68f761b8619a9ce74faff4ae0f0d9f9145b28bcb&selectedJob=218386242.

The good part is that I can reproduce it locally with the patches on that push applied and `./mach mochitest test_messageChannelWithMessageManager.xul`. Here's a rollup version of those patches: https://github.com/bgrins/firefox-patches/blob/ce3bb7fe3e50a8a2852df8137f23b5629882cf05/rollup-browser-dexbl.

Some observations I've had so far:

* If I don't call the `loadContext` getter, which in turn calls `nsFrameLoader::LoadContext`, which in turn calls `TabParent::GetLoadContext`, then the leak goes away. I can confirm this by commenting out this line in JS: https://github.com/bgrins/firefox-patches/blob/ce3bb7fe3e50a8a2852df8137f23b5629882cf05/rollup-browser-dexbl#L1810.
* The test uses this `<browser />`: https://searchfox.org/mozilla-central/rev/413dd3a1fab3446749f3690d93ee17e3196f8c37/dom/messagechannel/tests/mm_messageChannelParent.xul#11. If I remove [remote=true] then the leak goes away.
* It's not something particular to the assertions in the test - even if I comment the entirety of `prepare_test` at https://searchfox.org/mozilla-central/rev/413dd3a1fab3446749f3690d93ee17e3196f8c37/dom/messagechannel/tests/mm_messageChannelParent.js#127 and replace it with what the `finish` function there does (`opener.setTimeout("done()", 0); window.close();`) then it still leaks.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 21

4 months ago
It's worth noting that the call from JS to `frameLoader.loadContext` happens early during the test (when the <browser> is connected to the DOM), and not during window unload or browser shutdown.
I don't know exactly what the problem is, but some observations for future investigations...

* Removing the call to .loadContext/nsFrameLoader::LoadContext and/or removing [remote=true] on <browser/> suggests to me that something is going amiss here:

https://searchfox.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#3018-3019

We're creating `mRemoteBrowser` in the failure case (?) and that's causing other things to go wrong.

* We're leaking a tab group, which is kind of odd, but that's probably (?) what's holding on to the ThrottledEventQueues:

https://searchfox.org/mozilla-central/source/dom/base/TabGroup.cpp#83-85

Notice that we're leaking two of them, exactly how many we create in that loop.

* The SchedulerEventTarget leaks probably come from the TabGroup as well, since TabGroup inherits from SchedulerGroup:

https://searchfox.org/mozilla-central/source/dom/base/TabGroup.h#48-49

and SchedulerGroup holds on to a bunch of event targets:

https://searchfox.org/mozilla-central/source/xpcom/threads/SchedulerGroup.h#219

created in:

https://searchfox.org/mozilla-central/source/xpcom/threads/SchedulerGroup.cpp#172-182
https://searchfox.org/mozilla-central/source/xpcom/threads/SchedulerGroup.cpp#198-203

called from:

https://searchfox.org/mozilla-central/source/dom/base/TabGroup.cpp#41

* I don't really understand what *kind* of TabGroup we're leaking here.  I would assume a *chrome* TabGroup (one would think we're not leaking content TabGroups at this point...), which means that we ought to not be creating SchedulerEventTarget objects here:

https://searchfox.org/mozilla-central/source/xpcom/threads/SchedulerGroup.cpp#175-179

but leaking a chrome tab group doesn't line up with leaking SchedulerEventTargets...

* There are interesting comments around all of the above observations:

https://searchfox.org/mozilla-central/source/dom/base/TabGroup.cpp#43
https://searchfox.org/mozilla-central/source/dom/base/TabGroup.cpp#186

but none of those line up with leaking a non-chrome tab group.

I'm not sure that helps in any way...you can try running a local debug build with:

XPCOM_MEM_LOG_CLASSES=TabGroup ./mach mochitest ...

(cf. https://developer.mozilla.org/en-US/docs/Mozilla/Performance/BloatView#Bloat_Statistics_on_Tinderbox)

so we could find out where the offending TabGroup is being allocated, at least.  But logging individual classes doesn't get used very much and therefore might be broken...
Flags: needinfo?(nfroyd)
(Assignee)

Comment 23

4 months ago
(In reply to Nathan Froyd [:froydnj] [away until 2 January 2019] from comment #22)
> I don't know exactly what the problem is, but some observations for future
> investigations...

Thank you so much! I'll look into some of those calls.

> I'm not sure that helps in any way...you can try running a local debug build
> with:
> 
> XPCOM_MEM_LOG_CLASSES=TabGroup ./mach mochitest ...
> 
> (cf.
> https://developer.mozilla.org/en-US/docs/Mozilla/Performance/
> BloatView#Bloat_Statistics_on_Tinderbox)
> 
> so we could find out where the offending TabGroup is being allocated, at
> least.  But logging individual classes doesn't get used very much and
> therefore might be broken...

This is the output I get from that:

0:53.22 INFO == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, tab process 30225
 0:53.22 INFO 
 0:53.22 INFO      |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
 0:53.22 INFO      |                                      | Per-Inst   Leaked|   Total      Rem|
 0:53.22 INFO    0 |TOTAL                                 |       45     1808|   19264       19|
 0:53.22 INFO   41 |CondVar                               |       80      160|      52        2|
 0:53.22 INFO  124 |Mutex                                 |      104      208|     300        2|
 0:53.22 INFO  215 |SchedulerEventTarget                  |       48      432|      18        9|
 0:53.22 INFO  256 |TabGroup                              |      392      392|       1        1|
 0:53.22 INFO  264 |ThrottledEventQueue                   |       40       80|       2        2|
 0:53.22 INFO  265 |ThrottledEventQueue::Inner            |      264      528|       2        2|
 0:53.22 INFO  436 |nsTArray_base                         |        8        8|    4831        1|
 0:53.22 INFO 
 0:53.22 INFO nsTraceRefcnt::DumpStatistics: 471 entries
 0:53.22 INFO 
 0:53.22 INFO Serial Numbers of Leaked Objects:
 0:53.22 INFO 1 @0x109e9b1e0 (1 references; 0 from COMPtrs)
 0:53.22 INFO allocation stack:
15:41.69 INFO #00: NS_LogAddRef (nsTraceRefcnt.cpp:913, in XUL)
15:41.69 INFO #01: mozilla::dom::TabGroup::AddRef() (TabGroup.h:66, in XUL)
15:41.69 INFO #02: mozilla::RefPtrTraits<mozilla::SchedulerGroup>::AddRef(mozilla::SchedulerGroup*) (RefPtr.h:44, in XUL)
15:41.69 INFO #03: RefPtr<mozilla::SchedulerGroup>::ConstRemovingRefPtrTraits<mozilla::SchedulerGroup>::AddRef(mozilla::SchedulerGroup*) (RefPtr.h:362, in XUL)
15:41.69 INFO #04: RefPtr<mozilla::SchedulerGroup>::RefPtr(mozilla::SchedulerGroup*) (RefPtr.h:106, in XUL)
15:41.69 INFO #05: RefPtr<mozilla::SchedulerGroup>::RefPtr(mozilla::SchedulerGroup*) (RefPtr.h:106, in XUL)
15:41.69 INFO #06: (anonymous namespace)::SchedulerEventTarget::SchedulerEventTarget(mozilla::SchedulerGroup*, mozilla::TaskCategory) (SchedulerGroup.cpp:43, in XUL)
15:41.69 INFO #07: (anonymous namespace)::SchedulerEventTarget::SchedulerEventTarget(mozilla::SchedulerGroup*, mozilla::TaskCategory) (SchedulerGroup.cpp:43, in XUL)
15:41.69 INFO #08: mozilla::SchedulerGroup::CreateEventTargetFor(mozilla::TaskCategory) (SchedulerGroup.cpp:201, in XUL)
15:41.69 INFO #09: mozilla::SchedulerGroup::CreateEventTargets(bool) (SchedulerGroup.cpp:181, in XUL)
15:41.69 INFO #10: mozilla::dom::TabGroup::TabGroup(bool) (TabGroup.cpp:46, in XUL)
15:41.69 INFO #11: mozilla::dom::TabGroup::TabGroup(bool) (TabGroup.cpp:57, in XUL)
15:41.69 INFO #12: mozilla::dom::nsIContentChild::GetConstructedEventTarget(IPC::Message const&) (nsIContentChild.cpp:197, in XUL)
15:41.69 INFO #13: mozilla::dom::ContentChild::GetConstructedEventTarget(IPC::Message const&) (ContentChild.cpp:3226, in XUL)
15:41.69 INFO #14: mozilla::ipc::IToplevelProtocol::ToplevelState::GetMessageEventTarget(IPC::Message const&) (ProtocolUtils.cpp:833, in XUL)
15:41.69 INFO #15: mozilla::ipc::IToplevelProtocol::GetMessageEventTarget(IPC::Message const&) (ProtocolUtils.h:574, in XUL)
15:41.69 INFO #16: mozilla::ipc::MessageChannel::MessageTask::Post() (MessageChannel.cpp:2001, in XUL)
15:41.69 INFO #17: mozilla::ipc::MessageChannel::OnMessageReceivedFromLink(IPC::Message&&) (MessageChannel.cpp:0, in XUL)
15:41.69 INFO #18: mozilla::ipc::ProcessLink::OnMessageReceived(IPC::Message&&) (MessageLink.cpp:253, in XUL)
15:41.69 INFO #19: IPC::Channel::ChannelImpl::ProcessIncomingMessages() (ipc_channel_posix.cc:0, in XUL)
15:41.69 INFO #20: IPC::Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int) (ipc_channel_posix.cc:812, in XUL)
15:41.69 INFO #21: base::MessagePumpLibevent::OnLibeventNotification(int, short, void*) (message_pump_libevent.cc:246, in XUL)
15:41.69 INFO #22: event_persist_closure (event.c:1580, in XUL)
15:41.69 INFO #23: event_process_active_single_queue (event.c:1640, in XUL)
15:41.69 INFO #24: event_process_active (event.c:1738, in XUL)
15:41.69 INFO #25: event_base_loop (event.c:1961, in XUL)
15:41.70 INFO #26: base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) (message_pump_libevent.cc:338, in XUL)
15:41.70 INFO #27: MessageLoop::RunInternal() (message_loop.cc:314, in XUL)
15:41.70 INFO #28: MessageLoop::RunHandler() (message_loop.cc:308, in XUL)
15:41.70 INFO #29: MessageLoop::Run() (message_loop.cc:289, in XUL)
15:41.70 INFO #30: base::Thread::ThreadMain() (thread.cc:195, in XUL)
15:41.70 INFO #31: ThreadFunc(void*) (platform_thread_posix.cc:40, in XUL)
15:41.78 INFO #32: _pthread_body (in libsystem_pthread.dylib) + 126
15:41.78 INFO #33: _pthread_start (in libsystem_pthread.dylib) + 70
15:41.78 INFO leakcheck: tab leaked 2 CondVar
15:41.78 INFO leakcheck: tab leaked 2 Mutex
15:41.78 INFO leakcheck: tab leaked 9 SchedulerEventTarget
15:41.78 INFO leakcheck: tab leaked 1 TabGroup
15:41.78 INFO leakcheck: tab leaked 2 ThrottledEventQueue
15:41.78 INFO leakcheck: tab leaked 2 ThrottledEventQueue::Inner
15:41.78 INFO leakcheck: tab leaked 1 nsTArray_base
15:41.78 UNEXPECTED-FAIL leakcheck: tab 1808 bytes leaked
(Assignee)

Comment 24

4 months ago
Given the lines from the trace in Comment 23:

15:41.69 INFO #06: (anonymous namespace)::SchedulerEventTarget::SchedulerEventTarget(mozilla::SchedulerGroup*, mozilla::TaskCategory) (SchedulerGroup.cpp:43, in XUL)
15:41.69 INFO #07: (anonymous namespace)::SchedulerEventTarget::SchedulerEventTarget(mozilla::SchedulerGroup*, mozilla::TaskCategory) (SchedulerGroup.cpp:43, in XUL)
15:41.69 INFO #08: mozilla::SchedulerGroup::CreateEventTargetFor(mozilla::TaskCategory) (SchedulerGroup.cpp:201, in XUL)
15:41.69 INFO #09: mozilla::SchedulerGroup::CreateEventTargets(bool) (SchedulerGroup.cpp:181, in XUL)
15:41.69 INFO #10: mozilla::dom::TabGroup::TabGroup(bool) (TabGroup.cpp:46, in XUL)
15:41.69 INFO #11: mozilla::dom::TabGroup::TabGroup(bool) (TabGroup.cpp:57, in XUL)

It looks like we actually _are_ hitting the content case (line 181, which then calls CreateEventTargetFor): https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/xpcom/threads/SchedulerGroup.cpp#181.

The line numbers in TabGroup.cpp don't make sense to me - they seem to point to an `if` statement and a closing brace. Is this expected?

https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/base/TabGroup.cpp#46
https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/base/TabGroup.cpp#57
Flags: needinfo?(nfroyd)
Those stack frames for TabGroup.cpp don't make any sense to me either.  Unless the compiler is trying to tell you that it folded code together, or something?

The whole stack trace is a little weird, isn't it?  We're trying to get an event target for a new actor (#14 in comment 23):

https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/ipc/glue/ProtocolUtils.cpp#828-834

But we don't *have* a previously existing event target, so we're trying to create one (#13):

https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/ipc/ContentChild.cpp#3216-3226

That code tells us we must be handling a PContent::Msg_PBrowserConstructor__ID.  (If we were handling something else, we wouldn't get to the nsIContentChild::GetConstructedEventTarget call.)  This kind of makes sense, given what we knew previously, that *not* creating remote browsers made the leak go away.  So apparently we are creating a remote browser at an inopportune time?

We wind up creating a TabGroup here (#12):

https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/dom/ipc/nsIContentChild.cpp#195-200

So that code will have a reference to the TabGroup, and something else deep within the scheduler code will have a reference to the tab group.  But the reference from the nsIContentChild code will eventually get passed out to:

https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/ipc/glue/MessageChannel.cpp#1999-2007

and will get dropped at the end of that function.  So nothing else (?) has a reference to the tab group, only the scheduler code, and we therefore leak.

I guess in the normal case of handling PBrowserConstructor, we must already have an event target:

https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/ipc/glue/ProtocolUtils.cpp#819

or the newly-created TabGroup is grabbed on to by some other code somehow?
Flags: needinfo?(nfroyd)
(Assignee)

Comment 26

4 months ago
This is interesting - if I delay the call to `construct()` (which does the `loadContext` getter) until after DOMContentLoaded the leak seems to go away. We have a mechanism for doing this in our base Custom Element class with `delayConnectedCallback()`, so I think it'd be landable with that. But I'd still like to know why this is happening and fix it, since it seems like an existing issue that could pop back up sometime in the future.
Attachment #9032283 - Attachment description: Bug 1441935 - WIP - Modifications on top of the generated Custom Element → Bug 1441935 - Modifications on top of the generated Custom Element
Attachment #9005029 - Attachment is obsolete: true
Attachment #9032283 - Attachment description: Bug 1441935 - Modifications on top of the generated Custom Element → Bug 1441935 - To fold: Modifications on top of the generated Custom Element

Hey Fallen, just a heads up that this binding removal is coming to a head. I'm not 100% sure what comm-central needs to do in order to use the new Custom Element - if that's not in the cards, at the very least, you might want to fork browser.xml.

Flags: needinfo?(philipp)
(Assignee)

Comment 29

4 months ago

(In reply to Mike Conley (:mconley) (:⚙️) from comment #28)

Hey Fallen, just a heads up that this binding removal is coming to a head. I'm not 100% sure what comm-central needs to do in order to use the new Custom Element - if that's not in the cards, at the very least, you might want to fork browser.xml.

In theory this should "just work" for TB since it will be loaded via customElements.js. In practice, it would be worth testing with the patches here applied to confirm nothing out of the ordinary happens. Also if anything extends the 'browser' binding that will need updating - although I see one in suite/ but none for TB): https://searchfox.org/comm-central/search?q=xml%23browser&path=.

(Assignee)

Updated

4 months ago
Blocks: 1518282
(Assignee)

Updated

4 months ago
Blocks: 1518287
Attachment #9032281 - Attachment description: Bug 1441935 - `hg cp toolkit/content/widgets/browser.xml toolkit/content/widgets/browser.js` → Bug 1441935 - `hg cp toolkit/content/widgets/browser.xml toolkit/content/widgets/browser-custom-element.js`
(Assignee)

Comment 32

4 months ago

The results in Comment 31 are surprising. I wouldn't expect either of these - and especially not (2) - to make things worse.

Trying a new base push to see if there's variation on these win7-32 builds even without any patches applied:

Base vs base: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&framework=1&showOnlyImportant=1

And with the patches applied compared to the new base:

  1. No changes https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&newProject=try&newRevision=0389eb8b80a51a7a7f636b55ba0e3b28617fd215&framework=1&showOnlyImportant=1
  2. No chnges https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&newProject=try&newRevision=2c861abb7cfdb6b1b89cc7bef55bfa3f86f9c65d&framework=1&showOnlyImportant=1
  3. Don't use the delayConnectedCallback: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&newProject=try&newRevision=8b32fe71a8aa72c1397ccdd863d963b1a779e6b5&framework=1&showOnlyImportant=1
  4. Don't load non-browser Custom Elements for chrome HTML docs: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&newProject=try&newRevision=621a3059d9ede6fa32ed503987a4fca889847da5&framework=1&showOnlyImportant=1

(In reply to Mike Conley (:mconley) (:⚙️) from comment #28)

Hey Fallen, just a heads up that this binding removal is coming to a head. I'm not 100% sure what comm-central needs to do in order to use the new Custom Element - if that's not in the cards, at the very least, you might want to fork browser.xml.

Thanks Mike! We should be fine, Arshad is doing our de-xbl work and given we're using a few other custom elements they should be loaded. I'm cc'ing him though so he is aware.

Flags: needinfo?(philipp)
(Assignee)

Comment 34

4 months ago

(In reply to Brian Grinstead [:bgrins] from comment #32)

The results in Comment 31 are surprising. I wouldn't expect either of these - and especially not (2) - to make things worse.

Trying a new base push to see if there's variation on these win7-32 builds even without any patches applied:

Base vs base: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=f2e9fedb224724380a09cddb840b3c7f543a6643&framework=1&showOnlyImportant=1

So the new base is significantly better in tp5o_scroll and tresize on win7-32 pgo, and significantly worse in tscrollx pgo windows10-64. This was pushed with the same try syntax with the same rev, so this is quite a moving target.

(Assignee)

Comment 35

4 months ago

Hm, so I did two more pushes this morning with a new m-c tip and get the following results:

Basically no change, except for a tsvg_static opt e10s stylo windows7-32 improvement which I'll chalk up to noise given Comment 34. Mike, I obviously don't want to land something with a regression, but I'm having trouble determining if there's an improvement, regression, or no change as a result of this patch. What do you think we should do?

Flags: needinfo?(mconley)
(Assignee)

Comment 36

4 months ago

Note: I'm pushing these with ./mach try fuzzy -q '!qr /-talos-e10s' --rebuild 6 --no-artifact

If Talos is being inconsistent, then I suggest we not worry about it, tbh.

Flags: needinfo?(mconley)
(Assignee)

Comment 38

3 months ago
See Also: → 1486507
(Assignee)

Comment 39

3 months ago

(In reply to Brian Grinstead [:bgrins] from comment #38)

Darn, a change to browser_browser_languages_subdialog.js landed in Bug 1486507 yesterday that fails with the patch queue applied:

As best I can tell, this is catching an issue where by removing the DOM node during window unload, we are preventing the "cancel" telemetry from firing when the tab is removed (due to this._frame.contentWindow being null when trying to call this._frame.contentWindow.close()).

Probably calling just destroy instead of removing the element is the right thing to do anyway (closer than the current XBL behavior), although when doing that I see a leak on browser/components/extensions/test/browser/browser_ext_windows_create_url.js in linux32 debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b78973d5bb32817bb6cb273ab9018bf889353ef&selectedJob=220690293. So going to look into that now.

(Assignee)

Comment 40

3 months ago

(In reply to Brian Grinstead [:bgrins] from comment #39)

Probably calling just destroy instead of removing the element is the right thing to do anyway (closer than the current XBL behavior), although when doing that I see a leak on browser/components/extensions/test/browser/browser_ext_windows_create_url.js in linux32 debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b78973d5bb32817bb6cb273ab9018bf889353ef&selectedJob=220690293. So going to look into that now.

So when just calling destroy we were getting past the !aBrowser.parentNode condition and doing the aBrowser.isSyntheticDocument getter (https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/browser/base/content/browser-fullZoom.js#349). This would throw due to contentDocument being null, and presumably the leak is related to that (perhaps aCallback not being called). I confirmed locally that changing that condition to !aBrowser.mInitialized makes the leak go away. Did a try push to confirm that, and now going to see why we don't currently leak with the XBL version (which presumably still has a parent node as well).

(Assignee)

Comment 41

3 months ago

(In reply to Brian Grinstead [:bgrins] from comment #40)

(In reply to Brian Grinstead [:bgrins] from comment #39)

Probably calling just destroy instead of removing the element is the right thing to do anyway (closer than the current XBL behavior), although when doing that I see a leak on browser/components/extensions/test/browser/browser_ext_windows_create_url.js in linux32 debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b78973d5bb32817bb6cb273ab9018bf889353ef&selectedJob=220690293. So going to look into that now.

So when just calling destroy we were getting past the !aBrowser.parentNode condition and doing the aBrowser.isSyntheticDocument getter (https://searchfox.org/mozilla-central/rev/5053031ba7621fa8f63f42de4c204ab3561e4e59/browser/base/content/browser-fullZoom.js#349). This would throw due to contentDocument being null, and presumably the leak is related to that (perhaps aCallback not being called). I confirmed locally that changing that condition to !aBrowser.mInitialized makes the leak go away. Did a try push to confirm that, and now going to see why we don't currently leak with the XBL version (which presumably still has a parent node as well).

Ohh, so the .parentNode lookup seems just wrong for both CE and XBL, but the thing is with XBL isSyntheticDocument becomes undefined once it's destructed (so it never calls the property at https://searchfox.org/mozilla-central/rev/c3ebaf6de2d481c262c04bb9657eaf76bf47e2ac/toolkit/content/widgets/browser.xml#630 which would cause the error).

I think the right fix here is to check for mInitialized rather than checking the parent node (if there's no parent node then we won't have mInitialized set anyway).

Attachment #9032282 - Attachment description: Bug 1441935 - Import generated MozBrowser Custom Element implementation → Bug 1441935 - Copy browser.xml and import the generated MozBrowser Custom Element
Attachment #9032283 - Attachment description: Bug 1441935 - To fold: Modifications on top of the generated Custom Element → Bug 1441935 - Modifications on top of the generated MozBrowser Custom Element
Attachment #9032281 - Attachment is obsolete: true

Comment 42

3 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e943c6de64e
Copy browser.xml and import the generated MozBrowser Custom Element r=mconley
https://hg.mozilla.org/integration/autoland/rev/bd38d0bde8a3
Modifications on top of the generated MozBrowser Custom Element r=mconley
https://hg.mozilla.org/integration/autoland/rev/b342c8e8306e
Remove browser XBL binding r=mconley

Comment 43

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
(Assignee)

Updated

3 months ago
Blocks: 1519394
(Assignee)

Updated

3 months ago
Depends on: 1519461

(In reply to Brian Grinstead [:bgrins] from comment #2)

We may be able to
remove the clickthrough attribute altogether by changing
NodeAllowsClickThrough to return false in the case of browser/tree elements
and true for a scrollbar.

Did this change end up being made? As far as I can tell, the patches that landed removed the clickthrough attribute without replacement, which means that web content in background windows now responds to mouse events.

Comment 45

3 months ago

(In reply to Markus Stange [:mstange] from comment #44)

(In reply to Brian Grinstead [:bgrins] from comment #2)

We may be able to
remove the clickthrough attribute altogether by changing
NodeAllowsClickThrough to return false in the case of browser/tree elements
and true for a scrollbar.

Did this change end up being made? As far as I can tell, the patches that landed removed the clickthrough attribute without replacement, which means that web content in background windows now responds to mouse events.

+ni for this...

Flags: needinfo?(bgrinstead)
(Assignee)

Comment 46

3 months ago

(In reply to Markus Stange [:mstange] from comment #44)

(In reply to Brian Grinstead [:bgrins] from comment #2)

We may be able to
remove the clickthrough attribute altogether by changing
NodeAllowsClickThrough to return false in the case of browser/tree elements
and true for a scrollbar.

Did this change end up being made? As far as I can tell, the patches that landed removed the clickthrough attribute without replacement, which means that web content in background windows now responds to mouse events.

Looks like I missed this, thanks for the reminder! To be sure: the STR here is that when you have two windows and you click on something in the unfocused window we shouldn't process that as a click on the web content, but only focus the window?

Flags: needinfo?(bgrinstead)
(Assignee)

Updated

3 months ago
Flags: needinfo?(mstange)

That's right. And hovering things shouldn't apply :hover etc. As you said, making NodeAllowsClickThrough return false for browsers (either by adding the clickthrough attribute to the <browser> elements that we create, or through a new check in that function) should take care of this.

Flags: needinfo?(mstange)
(Assignee)

Updated

3 months ago
Depends on: 1519905
(Assignee)

Updated

3 months ago
Blocks: 1497799
Depends on: 1519091
Depends on: 1519078

Updated

3 months ago
Blocks: 1450382
Duplicate of this bug: 1502909
You need to log in before you can comment on or make changes to this bug.