1441935 - Convert <browser> XBL binding to a Custom Element

Status: RESOLVED FIXED

(Core :: XUL, task, P5)

Description

[Brian Grinstead [:bgrins]]

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 {}

Comment 1

[Brian Grinstead [:bgrins]]

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

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[Tim Nguyen :ntim]

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.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 6

[Tim Nguyen :ntim]

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.

Comment 7

[Brian Grinstead [:bgrins]]

(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

Comment 8

[Tim Nguyen :ntim]

Unassigning to reflect real status.

Comment 9

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336

Comment 10

[Brian Grinstead [:bgrins]]

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.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: Bug 1441935 - Import generated <browser> Custom Element implementation

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: Bug 1441935 - Fold together remote browser and browser classes

Also use a plain object as the observer

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: Bug 1441935 - Remove XBL bindings for browser and tabbrowser-browser

Comment 14

[Brian Grinstead [:bgrins]]

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.

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: Bug 1441935 - `hg cp toolkit/content/widgets/browser.xml toolkit/content/widgets/browser-custom-element.js`

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: Bug 1441935 - Copy browser.xml and import the generated MozBrowser Custom Element

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: Bug 1441935 - Modifications on top of the generated MozBrowser Custom Element

Steps to build:

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

Comment 18

[Brian Grinstead [:bgrins]]

Current try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36ee89953e6ac4069bf8ecaf8212777519e86c6d

Comment 19

[Brian Grinstead [:bgrins]]

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

Comment 20

[Brian Grinstead [:bgrins]]

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.

Comment 21

[Brian Grinstead [:bgrins]]

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.

Comment 22

[Nathan Froyd [:froydnj]]

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

Comment 23

[Brian Grinstead [:bgrins]]

(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

Comment 24

[Brian Grinstead [:bgrins]]

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

Comment 25

[Nathan Froyd [:froydnj]]

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?

Comment 26

[Brian Grinstead [:bgrins]]

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.

Comment 27

[Brian Grinstead [:bgrins]]

Attachment: Bug 1441935 - Remove browser XBL binding

Comment 28

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 29

[Brian Grinstead [:bgrins]]

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

Comment 30

[Brian Grinstead [:bgrins]]

I've done a handful of try pushes during development, but here's a full one with -p all -u all: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9d373214dc9820dab892bd7e89f3b52f1330bf0

I've also seen some semi-unexpected talos results from previous pushes, so I've done two separate ones with the same base to see how stable the results are:

1. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=0389eb8b80a51a7a7f636b55ba0e3b28617fd215&framework=1&showOnlyImportant=1
2. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=2c861abb7cfdb6b1b89cc7bef55bfa3f86f9c65d&framework=1&showOnlyImportant=1

Comment 31

[Brian Grinstead [:bgrins]]

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

I've also seen some semi-unexpected talos results from previous pushes, so I've done two separate ones with the same base to see how stable the results are:

1. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=0389eb8b80a51a7a7f636b55ba0e3b28617fd215&framework=1&showOnlyImportant=1
2. https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=2c861abb7cfdb6b1b89cc7bef55bfa3f86f9c65d&framework=1&showOnlyImportant=1

Hm, seeing a few new Windows7-32 regressions on those. Trying two ideas to see if they will help:

1. Don't use the delayConnectedCallback: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=8b32fe71a8aa72c1397ccdd863d963b1a779e6b5&framework=1&showOnlyImportant=1
2. Don't load non-browser Custom Elements for chrome HTML docs: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5ad6871711b65ed37f7f6e84f400c8efb1712293&newProject=try&newRevision=621a3059d9ede6fa32ed503987a4fca889847da5&framework=1&showOnlyImportant=1

Comment 32

[Brian Grinstead [:bgrins]]

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

Comment 33

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

Comment 34

[Brian Grinstead [:bgrins]]

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

Comment 35

[Brian Grinstead [:bgrins]]

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

• https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=231091724dbf5fec5712e83bf39f9da5c8b91483&newProject=try&newRevision=d1545452d4c2d2b24ecc1b02f52c800fc34ce1bc&framework=1&showOnlyImportant=1
• https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=231091724dbf5fec5712e83bf39f9da5c8b91483&newProject=try&newRevision=72f489d20f06d089eb968d76db29929c7511f851&framework=1&showOnlyImportant=1

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?

Comment 36

[Brian Grinstead [:bgrins]]

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

Comment 37

[Mike Conley (:mconley) (:⚙️)]

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

Comment 38

[Brian Grinstead [:bgrins]]

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

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=d14f6ce7f9183a361c6ac9be530988aab1089be0&selectedJob=220655302
• https://hg.mozilla.org/mozilla-central/rev/a94a05a0a11d

Comment 39

[Brian Grinstead [:bgrins]]

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

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=d14f6ce7f9183a361c6ac9be530988aab1089be0&selectedJob=220655302
• https://hg.mozilla.org/mozilla-central/rev/a94a05a0a11d

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.

Comment 40

[Brian Grinstead [:bgrins]]

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

Comment 41

[Brian Grinstead [:bgrins]]

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

Comment 42

[Pulsebot]

Pushed by [e-mail]:
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

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/4e943c6de64e
https://hg.mozilla.org/mozilla-central/rev/bd38d0bde8a3
https://hg.mozilla.org/mozilla-central/rev/b342c8e8306e

Comment 44

[Markus Stange [:mstange]]

(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

[:Gijs (he/him)]

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

Comment 46

[Brian Grinstead [:bgrins]]

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

Comment 47

[Markus Stange [: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.