Closed Bug 1441935 Opened 7 years ago Closed 6 years ago

Convert <browser> XBL binding to a Custom Element

Categories

(Core :: XUL, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

Attachments

(3 files, 4 obsolete files)

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 {}
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"); }
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.
Assignee: nobody → ntim.bugs
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.
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)
(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)
Depends on: 1444760
Depends on: 1445096
Depends on: 1445099
Depends on: 1445100
Priority: -- → P5
Unassigning to reflect real status.
Assignee: ntim.bugs → nobody
Component: XP Toolkit/Widgets: XUL → XUL
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: 1478372
Depends on: 1483274
Depends on: 1484326
Depends on: 1486079
Also use a plain object as the observer
Depends on: 1492967
Attachment #9005030 - Attachment is obsolete: true
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
Depends on: 1494103
Attachment #9005031 - Attachment is obsolete: true
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.
Depends on: 1496067
Depends on: 1492326
Steps to build: this.observer instead of this ./mach eslint toolkit/content/widgets/browser.js --fix
Down to (I think) one test failure - a leak with `./mach mochitest test_messageChannelWithMessageManager.xul`
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)
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)
(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
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)
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)

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

Blocks: 1518282
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`

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)

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

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)

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)

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

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

(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
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Blocks: 1519394
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.

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

(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)
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)
Depends on: 1519905
Blocks: 1497799
Depends on: 1519091
Depends on: 1519078
Blocks: 1450382
Blocks: 1437247
Type: enhancement → task
Regressions: 1760791
No longer regressions: 1760791
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: