Closed Bug 1350642 Opened 8 years ago Closed 7 years ago

Remove the PBrowser::Msg_GetTabCount sync IPC

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: mrbkap)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 6 obsolete files)

Bug 1350642 - Remove the PBrowser::Msg_GetTabCount sync IPC; r=krizsa
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
Bug 1350642 - Keep the tabCountResizable property in sync; r?dao
46 bytes, text/x-phabricator-request
dao
: review+
Details | Review
This was added by bug 1258925. The median time according to telemetry data is 2.5ms, so this isn't our worst sync IPC, but still, can we do something more efficient here, given that this can be triggered from a web page?
Haik, can you comment on whether or not we can do something more efficient?
Flags: needinfo?(haftandilian)
Priority: -- → P2
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
(In reply to Andrew Overholt [:overholt] from comment #1) > Haik, can you comment on whether or not we can do something more efficient? Gabor was the reviewer of bug 1258952. Perhaps he has ideas, too.
Flags: needinfo?(gkrizsanits)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2) > (In reply to Andrew Overholt [:overholt] from comment #1) > > Haik, can you comment on whether or not we can do something more efficient? > > Gabor was the reviewer of bug 1258952. Perhaps he has ideas, too. ^^^^^^^^^^^^^^ oops, typo: should be bug 1258925
(In reply to Andrew Overholt [:overholt] from comment #1) > Haik, can you comment on whether or not we can do something more efficient? Yes, I think we could do something more efficient by moving the TabCount part of the window-resize-security-check into the parent. For bug 1258925 "[e10s] Browser window is resized when click", we added the sync message so that that content could use the number of tabs in nsGlobalWindow::CanMoveResizeWindows. Since the parent performs the resize, we should be able to move the TabCount part of the check to the parent.
Assignee: nobody → haftandilian
Flags: needinfo?(haftandilian)
Flags: needinfo?(gkrizsanits)
(In reply to Haik Aftandilian [:haik] from comment #4) > (In reply to Andrew Overholt [:overholt] from comment #1) > > Haik, can you comment on whether or not we can do something more efficient? > > Yes, I think we could do something more efficient by moving the TabCount > part of the window-resize-security-check into the parent. For bug 1258925 > "[e10s] Browser window is resized when click", we added the sync message so > that that content could use the number of tabs in > nsGlobalWindow::CanMoveResizeWindows. Since the parent performs the resize, > we should be able to move the TabCount part of the check to the parent. I doubt that would work actually, since at that point the puppet widget in the child would be resized and would be left in an inconsistent state with the parent. I think a better solution would be to notify each TabChild when a tab is added/removed, so that they can maintain a precise tab count themselves. The parent will be able to send these notifications asynchronously since the child won't need to know this at a precise moment in time. This can replace the existing sync IPC message, I think.
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #5) > (In reply to Haik Aftandilian [:haik] from comment #4) > > (In reply to Andrew Overholt [:overholt] from comment #1) > > > Haik, can you comment on whether or not we can do something more efficient? > > > > Yes, I think we could do something more efficient by moving the TabCount > > part of the window-resize-security-check into the parent. For bug 1258925 > > "[e10s] Browser window is resized when click", we added the sync message so > > that that content could use the number of tabs in > > nsGlobalWindow::CanMoveResizeWindows. Since the parent performs the resize, > > we should be able to move the TabCount part of the check to the parent. > > I doubt that would work actually, since at that point the puppet widget in > the child would be resized and would be left in an inconsistent state with > the parent. I think a better solution would be to notify each TabChild when > a tab is added/removed, so that they can maintain a precise tab count > themselves. The parent will be able to send these notifications > asynchronously since the child won't need to know this at a precise moment > in time. This can replace the existing sync IPC message, I think. It looks like TabChild::RecvUpdateDimensions is how the resize result is sent to the child after the child calls PBrowserChild::SendSetDimensions. And that triggers PuppetWidget::Resize. Then as long as TabChild::RecvUpdateDimensions is reliable, the "inconsistent state" scenario should not occur. With some quick debugging, on a resize triggered from content, I found PuppetWidget::Resize was called in stacks from TabChild::RecvRealMouseButtonEvent and TabChild::RecvUpdateDimensions, but I will look into this more. And I'll post the hack code I've been working on to the tab count check to the parent. In general, given that we're sending an async message to the parent to do the resize, wouldn't it be possible for the window dimensions to already be out-of-date by the time the parent receives the SendSetDimensions message? For example, another tab could have resized the window already. So we have to rely on the parent giving us the actual updated dimensions after a resize request anyway. Example PBrowserParent::RecvUpdateDimensions stack originating from TabParent::RecvSetDimensions: frame #0 XUL`mozilla::dom::PBrowserParent::SendUpdateDimensions() at PBrowserParent.cpp:384 frame #1 XUL`mozilla::dom::TabParent::UpdateDimensions() at TabParent.cpp:787 frame #2 XUL`nsFrameLoader::UpdatePositionAndSize() at nsFrameLoader.cpp:2696 frame #3 XUL`nsSubDocumentFrame::ReflowFinished() at nsSubDocumentFrame.cpp:831 frame #4 XUL`non-virtual thunk to nsSubDocumentFrame::ReflowFinished()() at nsSubDocumentFrame.cpp:0 frame #5 XUL`mozilla::PresShell::HandlePostedReflowCallbacks() at PresShell.cpp:4011 frame #6 XUL`mozilla::PresShell::DidDoReflow() at PresShell.cpp:9142 frame #7 XUL`mozilla::PresShell::ResizeReflowIgnoreOverride() at PresShell.cpp:1992 frame #8 XUL`mozilla::PresShell::ResizeReflow() at PresShell.cpp:1920 frame #9 XUL`nsViewManager::DoSetWindowDimensions() at nsViewManager.cpp:192 frame #10 XUL`nsViewManager::SetWindowDimensions() at nsViewManager.cpp:228 frame #11 XUL`nsView::WindowResized() at nsView.cpp:1018 frame #12 XUL`nsChildView::ReportSizeEvent() at nsChildView.mm:1577 frame #13 XUL`nsChildView::Resize() at nsChildView.mm:1047 frame #14 XUL`nsDocumentViewer::SetBoundsWithFlags() at nsDocumentViewer.cpp:2006 frame #15 XUL`nsDocShell::SetPositionAndSize() at nsDocShell.cpp:6011 frame #16 XUL`non-virtual thunk to nsDocShell::SetPositionAndSize(int, int, int, int, unsigned int)() at nsDocShell.cpp:0 frame #17 XUL`nsWebShellWindow::WindowResized() at nsWebShellWindow.cpp:307 frame #18 XUL`non-virtual thunk to nsWebShellWindow::WindowResized(nsIWidget*, int, int)() at nsWebShellWindow.cpp:0 frame #19 XUL`nsCocoaWindow::ReportSizeEvent() at nsCocoaWindow.mm:1980 frame #20 XUL`::-[WindowDelegate windowDidResize:](NSNotification *)() at nsCocoaWindow.mm:2437 frame #21 CoreFoundation`__CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__() frame #22 CoreFoundation`_CFXRegistrationPost() frame #23 CoreFoundation`___CFXNotificationPost_block_invoke() frame #24 CoreFoundation`-[_CFXNotificationRegistrar find:object:observer:enumerator:]() frame #25 CoreFoundation`_CFXNotificationPost() frame #26 Foundation`-[NSNotificationCenter postNotificationName:object:userInfo:]() frame #27 AppKit`-[NSWindow _setFrameCommon:display:stashSize:]() frame #28 AppKit`-[NSWindow _setFrame:display:allowImplicitAnimation:stashSize:]() frame #29 AppKit`-[NSWindow setFrame:display:]() frame #30 XUL`nsCocoaWindow::DoResize() at nsCocoaWindow.mm:1593 frame #31 XUL`nsCocoaWindow::Resize() at nsCocoaWindow.mm:1612 frame #32 XUL`nsXULWindow::SetSize() at nsXULWindow.cpp:597 frame #33 XUL`nsChromeTreeOwner::SetSize() at nsChromeTreeOwner.cpp:334 frame #34 XUL`non-virtual thunk to nsChromeTreeOwner::SetSize(int, int, bool, unsigned int)() at nsChromeTreeOwner.cpp:0 frame #35 XUL`mozilla::dom::TabParent::RecvSetDimensions() at TabParent.cpp:725 ...
Updated patch 2 to use nsContentUtils::GetCurrentJSContext() and nsContentUtils::IsSystemCaller() to determine if the code is running system or client JS. This is a variant of the original approach. But this results in failures in dom/tests/mochitest/bugs/test_resize_move_windows.html: 53 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/bugs/test_resize_move_windows.html | Window width shouldn't have changed - got 335, expected 1280 57 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/bugs/test_resize_move_windows.html | Window outerWidth shouldn't have changed - got 335, expected 1280 60 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/bugs/test_resize_move_windows.html | Window height shouldn't have changed - got 100, expected 712 64 INFO TEST-UNEXPECTED-FAIL | dom/tests/mochitest/bugs/test_resize_move_windows.html | Window outerHeight shouldn't have changed - got 177, expected 789 Needs more debugging to determine root cause.
I don't think the minimal approach I attempted is going to work (where I send a flag to the parent to indicate if the caller is system or content). I didn't debug the above failure, but I think this code from SetReplaceableWindowCoord() is a problem. nsGlobalWindow::SetReplaceableWindowCoord() if (!outer || !outer->CanMoveResizeWindows(aCallerType) || outer->IsFrame()) { RedefineProperty(aCx, aPropName, aValue, aError); return; } Here, even if CanMoveResizeWindows fails, we still set the property as if it was allowed. If we deferred the resize check to the parent, at the time SetReplaceableWindowCoord() is called in the child, we wouldn't be able to set the property correctly without possibly changing it later. If we set it to the provided value in SetReplaceableWindowCoord() and then it was set correctly later after the parent responds and allows the resize, we could set it twice to different values which would be detectable from content. I'm now looking into the approach Ehsan described in comment 5. When a new tab is added to or removed from a window, we'll have to send a message to every content process with a tab in that window. The content process will have to store the tabCount in each TabChild. The tabCount will need to be setup correctly on session restores too. It looks like we already have TabOpen and TabClose events that can be used.
Ehsan or Smaug, would either of you be able to point me in the right direction on this? I can see how I can use the "TabOpen" event to see any time a tab is opened on a window and get the number of tabs. For example, gBrowser.tabContainer.addEventListener("TabOpen", function() { window.messageManager.broadcastAsyncMessage("Browser:TabCount", {gBrowser.tabs.length}); }); But I'm not sure how to iterate over the TabChild's in a content process that are in the current window.
Flags: needinfo?(ehsan)
Flags: needinfo?(bugs)
Why would you need to iterate the TabChild objects if you use window.messageManager to broadcast the message. It already sends the message only to the TabChild objects in that window.
Flags: needinfo?(bugs)
Attachment #8865640 - Attachment is obsolete: true
(In reply to Olli Pettay [:smaug] from comment #14) > Why would you need to iterate the TabChild objects if you use > window.messageManager to broadcast the message. It already sends the message > only to the TabChild objects in that window. Thanks. I don't know if that was the right solution. Here's where I am with this bug. I've implemented the fix using window.messageManager.broadcastAsyncMessage() in the attached patch and it seems to work well and is passing on try. I have concerns about this approach because it requires sending one IPC message per tab in the window. For a window with 100 tabs, adding a tab or closing a tab will trigger the parent to send 100 IPC messages. It sends them as individual tasks dispatched to the main thread. (https://treeherder.mozilla.org/#/jobs?repo=try&revision=c830263d54a27144dcc3abe4fc75d612022edb58) As an improvement that generates less IPC traffic, I've tried to implement only sending messages when the number of tabs change from 1->2 or 2->1. I haven't got this to work yet. It fails on browser/components/sessionstore/test/browser_906076_lazy_tabs.js (https://treeherder.mozilla.org/#/jobs?repo=try&revision=00a56890d930c6e6d6ae55e05ea4af576722f9f2). I haven't debugged this yet. When I asked about iterating TabChild objects, I was thinking it would be better to minimize how many messages the parent has to send by sending a single message to each content process running a tab in the window. The content process would then iterate TabChild objects in the relevant window and update those TabChilds. That way we send a maximum of dom.ipc.processCount messages. Olli, do you have any comments or recommendations?
Flags: needinfo?(bugs)
(In reply to Haik Aftandilian [:haik] from comment #16) > As an improvement that generates less IPC traffic, I've tried to implement > only sending messages when the number of tabs change from 1->2 or 2->1. I > haven't got this to work yet. It fails on > browser/components/sessionstore/test/browser_906076_lazy_tabs.js > (https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=00a56890d930c6e6d6ae55e05ea4af576722f9f2). I haven't > debugged this yet. I guess you may want to check `if (lastTab.linkedBrowser.isConnected)` before sending the message, and then maybe send the message later on "TabBrowserInserted" or so.
So one would need to know which all child processes are being used, then send message to those child processes and somehow in child side map the message to the right tab childs. I guess one could collect arrays of TabParent objects and send them to child. Not sure if that is easily available on JS already.
Flags: needinfo?(bugs)
(In reply to Haik Aftandilian [:haik] from comment #13) > Ehsan or Smaug, would either of you be able to point me in the right > direction on this? (Looks like Olli responsed, clearing my ni)
Flags: needinfo?(ehsan)
(In reply to Samael Wang [:freesamael] from comment #17) > (In reply to Haik Aftandilian [:haik] from comment #16) > > As an improvement that generates less IPC traffic, I've tried to implement > > only sending messages when the number of tabs change from 1->2 or 2->1. I > > haven't got this to work yet. It fails on > > browser/components/sessionstore/test/browser_906076_lazy_tabs.js > > (https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=00a56890d930c6e6d6ae55e05ea4af576722f9f2). I haven't > > debugged this yet. > > I guess you may want to check `if (lastTab.linkedBrowser.isConnected)` > before sending the message, and then maybe send the message later on > "TabBrowserInserted" or so. Thanks :freesamael, I implemented that and it's working well. I'll flag you for review, but let me know if you're not the right person. My understanding is that lazy tabs are the tabs that are used after a session restore when a tab isn't loaded until it is clicked. And for those tabs, tab.linkedBrowser.isConnected==false. Is that right? If that's the case, with my current fix, lazy tabs won't be sent the TabCountResizable(false) message. However, such a tab wouldn't be allowed to be resized by non-system code because it wasn't opened with window.open() so it doesn't cause problems.
(In reply to Haik Aftandilian [:haik] from comment #21) > My understanding is that lazy tabs are the tabs that are used after a > session restore when a tab isn't loaded until it is clicked. And for those > tabs, tab.linkedBrowser.isConnected==false. Is that right? > > If that's the case, with my current fix, lazy tabs won't be sent the > TabCountResizable(false) message. However, such a tab wouldn't be allowed to > be resized by non-system code because it wasn't opened with window.open() so > it doesn't cause problems. This could probably be solved by additionally using the "TabSelect" event and sending the TabCountResizable message then.
(In reply to Haik Aftandilian [:haik] from comment #21) > Thanks :freesamael, I implemented that and it's working well. I'll flag you > for review, but let me know if you're not the right person. Sorry I'm not a module owner or peer so I'm technically unqualified :) But I can try to give you some feedbacks. > My understanding is that lazy tabs are the tabs that are used after a > session restore when a tab isn't loaded until it is clicked. And for those > tabs, tab.linkedBrowser.isConnected==false. Is that right? From what I understand, yes. The <xul:browser> element (along with some containers) is not appended to the document until tabbrowser._insertBrowser [1], so Node.isConnected [2] remains false until then. [1] http://searchfox.org/mozilla-central/rev/3b4f95ac5946b639827358d93b5258d837a5ddde/browser/base/content/tabbrowser.xml#2255 [2] https://dom.spec.whatwg.org/#dom-node-isconnected > This could probably be solved by additionally using the "TabSelect" event and sending the TabCountResizable message then. According to the comments [3], when a tab is selected it should also trigger TabBrowserInserted if it was a lazy browser. [3] http://searchfox.org/mozilla-central/rev/3b4f95ac5946b639827358d93b5258d837a5ddde/browser/base/content/tabbrowser.xml#6582-6585 You may want to split the tabbrowser/browser.js related changes into a separate patch and ask ::dao for reviewing.
Comment on attachment 8865639 [details] Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in sync; https://reviewboard.mozilla.org/r/136794/#review149538 ::: browser/base/content/browser.js:1416 (Diff revision 5) > > gBrowser.addEventListener("PermissionStateChange", function() { > gIdentityHandler.refreshIdentityBlock(); > }); > > + gBrowser.tabContainer.addEventListener("TabBrowserInserted", function(event) { If there's one existing tab, theoratically someone can invoke tabbrowser.addTab to create a lazy browser, then there'll be 2 tabs but the existing tab won't be notified for TabCountResizable since it will only cause a TabOpen event, not TabBrowserInserted yet. I don't know if that could happen in real world cases so I'm not sure if that should be handled. ::: browser/base/content/browser.js:1433 (Diff revision 5) > + > + // We either have 1 tab, in which case we don't block resizes due to > + // the tab count, or we have >2 tabs, in which case only the newest tab > + // needs to be told that the tab count doesn't permit resizes. > + let addedTab = event.detail.tab; > + if (addedTab.linkedBrowser.isConnected) { My understanding is that "TabBrowserInserted" happens when the <xul:browser> element is actually inserted into the document (i.e. it's no longer a lazy browser, or it's not created as a lazy browser in the first place), so I expect it's always connected when TabBrowserInserted event occurs and this check would be redundant. ::: browser/base/content/tabbrowser.xml:2253 (Diff revision 5) > // for non-remote browsers after we have called browser.loadURI(). > if (remoteType == E10SUtils.NOT_REMOTE) { > this._outerWindowIDBrowserMap.set(browser.outerWindowID, browser); > } > > - var evt = new CustomEvent("TabBrowserInserted", { bubbles: true, detail: {} }); > + var evt = new CustomEvent("TabBrowserInserted", { bubbles: true, detail: {tab: aTab} }); The event is dispatching on aTab, so instead of adding aTab as another parameter you can access it tho originalTarget like this: http://searchfox.org/mozilla-central/rev/3b4f95ac5946b639827358d93b5258d837a5ddde/browser/components/sessionstore/SessionStore.jsm#988,994
Attachment #8865639 - Flags: review?(sawang)
(In reply to Samael Wang [:freesamael] from comment #25) > Comment on attachment 8865639 [details] > Bug 1350642 - Remove the PBrowser::Msg_GetTabCount sync IPC; > > https://reviewboard.mozilla.org/r/136794/#review149538 > > ::: browser/base/content/browser.js:1416 > (Diff revision 5) > > > > gBrowser.addEventListener("PermissionStateChange", function() { > > gIdentityHandler.refreshIdentityBlock(); > > }); > > > > + gBrowser.tabContainer.addEventListener("TabBrowserInserted", function(event) { > > If there's one existing tab, theoratically someone can invoke > tabbrowser.addTab to create a lazy browser, then there'll be 2 tabs but the > existing tab won't be notified for TabCountResizable since it will only > cause a TabOpen event, not TabBrowserInserted yet. > > I don't know if that could happen in real world cases so I'm not sure if > that should be handled. Thanks for the feedback. I think that's the one issue that remains.
Comment on attachment 8865639 [details] Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in sync; https://reviewboard.mozilla.org/r/136794/#review150044 ::: browser/base/content/browser.js:1418 (Diff revision 7) > gIdentityHandler.refreshIdentityBlock(); > }); > > + gBrowser.tabContainer.addEventListener("TabBrowserInserted", function(aEvent) { > + // The new tab is already in gBrowser.tabs > + let newCount = gBrowser.tabs.length; Tabs can have browsers that aren't inserted, e.g. after session restore. In other words, you may not see a TabBrowserInserted event even though gBrowser.tabs is > 1.
Attachment #8865639 - Flags: review?(dao+bmo) → review-
Comment on attachment 8865639 [details] Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in sync; https://reviewboard.mozilla.org/r/136794/#review150080 ::: browser/base/content/browser.js:1435 (Diff revision 7) > + // the tab count, or we have >2 tabs, in which case only the newest tab > + // needs to be told that the tab count doesn't permit resizes. > + let addedTab = aEvent.originalTarget; > + addedTab.linkedBrowser > + .messageManager > + .sendAsyncMessage("Browser:TabCountResizable", newCount == 1); Can we block moving and resizing the window by default and only bother enabling it when a tab becomes the only tab? This would reduce the amount of messages front-end code has to send.
Comment on attachment 8865639 [details] Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in sync; https://reviewboard.mozilla.org/r/136794/#review150044 > Tabs can have browsers that aren't inserted, e.g. after session restore. In other words, you may not see a TabBrowserInserted event even though gBrowser.tabs is > 1. The code isn't dependent on seeing the TabBrowserInserted event for those tabs, just that they be in gBrowser.tabs. On a SessionRestore for a multi-tab window, we see the TabBrowserInserted for the selected tab and it finds gBrowser.tabs>1. The selected TabChild then gets sent the message that resizes are not permitted. When those restored tabs get selected, TabBrowserInserted is called.
Comment on attachment 8865639 [details] Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in sync; https://reviewboard.mozilla.org/r/136794/#review150080 > Can we block moving and resizing the window by default and only bother enabling it when a tab becomes the only tab? This would reduce the amount of messages front-end code has to send. I can look into that, but the TabOpen or TabBrowserInserted events don't fire when a new window is opened. I'll need to send a message to the TabChild when a new window is opened to tell it resizes are permitted. The current fix sends one message when a tab is added and one message when the tab count drops to 1.
(In reply to Haik Aftandilian [:haik] from comment #33) > Comment on attachment 8865639 [details] > Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in > sync; > > https://reviewboard.mozilla.org/r/136794/#review150044 > > > Tabs can have browsers that aren't inserted, e.g. after session restore. In other words, you may not see a TabBrowserInserted event even though gBrowser.tabs is > 1. > > The code isn't dependent on seeing the TabBrowserInserted event for those > tabs, just that they be in gBrowser.tabs. The point is that you won't even get to check gBrowser.tabs if you didn't get the TabBrowserInserted event in the first place. > On a SessionRestore for a > multi-tab window, we see the TabBrowserInserted for the selected tab and it > finds gBrowser.tabs>1. That's a session restore implementation detail. The tabbrowser API allows for having multiple tabs without a TabBrowserInserted event having been dispatched.
(In reply to Haik Aftandilian [:haik] from comment #34) > Comment on attachment 8865639 [details] > Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in > sync; > > https://reviewboard.mozilla.org/r/136794/#review150080 > > > Can we block moving and resizing the window by default and only bother enabling it when a tab becomes the only tab? This would reduce the amount of messages front-end code has to send. > > I can look into that, but the TabOpen or TabBrowserInserted events don't > fire when a new window is opened. I'll need to send a message to the > TabChild when a new window is opened to tell it resizes are permitted. > > The current fix sends one message when a tab is added and one message when > the tab count drops to 1. dao, thanks for the feedback. I haven't been able to find where to add code for this. When a new window is opened with a single tab, I need to send a message to the corresponding TabChild when it gets created. Do you know what would be the right place to do that?
Flags: needinfo?(dao+bmo)
(In reply to Haik Aftandilian [:haik] from comment #36) > When a new window is opened with a single tab, I need to send a > message to the corresponding TabChild when it gets created. Do you know what > would be the right place to do that? The first <xul:browser> of a <xul:tabbrowser> is declared statically at [1], and browser.js updates its remoteness if necessary later at [2]. It reminds me that we may have actually missed the case of remoteness changes - when navigating from a chrome-only URI such as about:config to web content which can be loaded in child process, tabbrowser would need to replace that <xul:browser remote="false"> with a <xul:browser remote="true"> instance (or vice versa). The process would destroy or create TabParent / TabChild pair, and use session restore to restore the history. It somewhat resembles opening a new (e10s) tab and closing the old (non-e10s) one, but without TabOpen / TabBrowserInserted, so its tabCountResizable won't be updated. Since we only care about remote browsers, I image you could add a listener on "TabRemotenessChange" that would handle both normal remoteness change and initBrowser cases (unless one day we decide to make initBrowser remote="true" by default...). [1] http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/browser/base/content/tabbrowser.xml#29-31 [2] http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/browser/base/content/browser.js#1244-1246
Comment on attachment 8874642 [details] Bug 1350642 - part 1 - Remove the PBrowser::Msg_GetTabCount sync IPC; https://reviewboard.mozilla.org/r/145986/#review151352 If the first patch makes sure that this property works as expected this part looks fine. ::: dom/interfaces/base/nsITabChild.idl:40 (Diff revision 2) > > + /* > + * Indicates whether or not the number of tabs in the same window as > + * this TabChild permits the window to be resized by non-system code. > + */ > + attribute boolean tabCountResizable; This property name sounds somewhat odd to me on nsITabChild. How about hasSiblings? Just an idea...
Attachment #8874642 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8874642 [details] Bug 1350642 - part 1 - Remove the PBrowser::Msg_GetTabCount sync IPC; https://reviewboard.mozilla.org/r/145986/#review151352 > This property name sounds somewhat odd to me on nsITabChild. How about hasSiblings? Just an idea... Thanks, that is a more intuitive name. Changed to hasSiblings.
The latest version of the patch makes hasSiblings=false the default. We only send a message over IPC when a new Window is created, when the number of tabs goes from 1->2 or 2->1, or when the remoteness changes.
Comment on attachment 8865639 [details] Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in sync; https://reviewboard.mozilla.org/r/136794/#review154450 ::: browser/base/content/browser.js:1389 (Diff revision 8) > + document.getAnonymousElementByAttribute(gBrowser, "anonid", "initialBrowser"); > + let initialTab = gBrowser.getTabForBrowser(initialBrowser); > + if (initialTab) { > + initialTab.linkedBrowser > + .messageManager > + .sendAsyncMessage("Browser:HasSiblings", false); I think this would probably fit better in tabbrowser's <constructor> ::: browser/base/content/browser.js:1434 (Diff revision 8) > + for (let tab of gBrowser.tabs) { > + if (tab !== aEvent.originalTarget) { > + tab.linkedBrowser > + .messageManager > + .sendAsyncMessage("Browser:HasSiblings", true); > + return; And this could just go in addTab? Alternatively, this code could be contained in its own object. I don't like the stray event listeners living directly in _delayedStartup.
Attachment #8865639 - Flags: review?(dao+bmo)
Flags: needinfo?(dao+bmo)
Hi Haik, are you still working on this bug? Is the patch in a landable state?
Flags: needinfo?(haftandilian)
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #44) > Hi Haik, are you still working on this bug? Is the patch in a landable state? It slipped off my radar due to higher priority tasks, but I'd like to get it into 57. I'm not sure how to address dao's feedback in comment comment 43. I'll work on that and either ask for help or get a new patch posted this week.
Flags: needinfo?(haftandilian)
(In reply to Haik Aftandilian [:haik] from comment #45) > I'm not sure how to address dao's feedback in comment comment 43. I think Dao's suggestion is that you can move everything you wrote in browser.js into tabbrowser.xml, so in addTab you can `sendAsyncMessage("Browser:HasSiblings", true)` and register per-tab "TabClose" & "TabRemotenessChange" event listeners. But initialBrowser and its tab isn't created by addTab, so you may also need to register listeners in the constructor somewhere after mCurrentTab is set at http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/browser/base/content/tabbrowser.xml#5457 (That makes me think why not just register listeners on tabContainer in the constructor, but I'm not sure if Dao would like this idea.)
ping again. I understand this isn't our worst sync IPC, but it looks like the patch only needs a little polish to land. I'd really like to get more testing before we branch off beta 57. Maybe Samael can also help to get this landed?
Flags: needinfo?(haftandilian)
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #47) > ping again. I understand this isn't our worst sync IPC, but it looks like > the patch only needs a little polish to land. I'd really like to get more > testing before we branch off beta 57. > > Maybe Samael can also help to get this landed? I'll be working on this today.
Flags: needinfo?(haftandilian)
Comment on attachment 8865639 [details] Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in sync; https://reviewboard.mozilla.org/r/136794/#review154450 > I think this would probably fit better in tabbrowser's <constructor> I wasn't able to get this to work in the <constructor>. > And this could just go in addTab? Alternatively, this code could be contained in its own object. I don't like the stray event listeners living directly in _delayedStartup. Moved to addTab.
(In reply to Samael Wang [:freesamael] from comment #46) > (In reply to Haik Aftandilian [:haik] from comment #45) > > I'm not sure how to address dao's feedback in comment comment 43. > > I think Dao's suggestion is that you can move everything you wrote in > browser.js into tabbrowser.xml, so in addTab you can > `sendAsyncMessage("Browser:HasSiblings", true)` and register per-tab > "TabClose" & "TabRemotenessChange" event listeners. But initialBrowser and > its tab isn't created by addTab, so you may also need to register listeners > in the constructor somewhere after mCurrentTab is set at > http://searchfox.org/mozilla-central/rev/ > 13148faaa91a1c823a7d68563d9995480e714979/browser/base/content/tabbrowser. > xml#5457 Thanks, Samael. Could you clarify what you mean by registering listeners in the constructor? Do you mean register a listener in the constructor that calls "...sendAsyncMessage("Browser:HasSiblings", false)" when the first tab is loaded? I've updated the code to add the tabContainer listeners in the tabbrowser constructor, but am still sending the initial HasSiblings=false message from _delayedStartup().
(In reply to Haik Aftandilian [:haik] from comment #52) > Thanks, Samael. Could you clarify what you mean by registering listeners in > the constructor? Do you mean register a listener in the constructor that > calls "...sendAsyncMessage("Browser:HasSiblings", false)" when the first tab > is loaded? > > I've updated the code to add the tabContainer listeners in the tabbrowser > constructor, but am still sending the initial HasSiblings=false message from > _delayedStartup(). What I thought was that either you register per-Tab listeners in both addTab & constructor, or you register tabContainer listeners in the constructor. It seems you've taken the latter approach. You probably don't really need to send the initial HasSiblings=false message since it only matters in case of <xul:browser remote="true">. My understanding is that initialBrowser is not a remote browser until gBrowserInit.onLoad updates its remoteness [1], and that should hit the "TabRemotenessChange" listener you added to send the message. [1] http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/browser/base/content/browser.js#1279-1281
Comment on attachment 8865639 [details] Bug 1350642 - part 2 - Use tab events to keep tabCountResizable property in sync; https://reviewboard.mozilla.org/r/136794/#review180496 ::: browser/base/content/browser.js:1410 (Diff revision 9) > > + let initialBrowser = > + document.getAnonymousElementByAttribute(gBrowser, "anonid", "initialBrowser"); > + let initialTab = gBrowser.getTabForBrowser(initialBrowser); > + if (initialTab) { > + initialTab.linkedBrowser initialTab.linkedBrowser is the same as initialBrowser here. The initialTab roundtrip doesn't seem to make sense. ::: browser/base/content/tabbrowser.xml:5702 (Diff revision 9) > > XPCOMUtils.defineLazyPreferenceGetter(this, "animationsEnabled", > "toolkit.cosmeticAnimations.enabled", true); > + > + // Add listeners for keeping HasSiblings in sync > + this.tabContainer.addEventListener("TabOpen", (function(aEvent) { The TabOpen, TabClose and TabRemotenessChange events are dispatched from tabbrowser.xml. This binding listening to its own events seems a bit silly. You should be able to do this stuff directly where the events are dispatched without the extra listeners.
Attachment #8865639 - Flags: review?(dao+bmo)
Whiteboard: [qf:p1] → [qf:p2]
I ran into an issue with the current approach. For a window.open() call followed by a resize such as with dom/tests/mochitest/general/test_resizeby.html: ... let popup = window.open("about:blank", "_blank", "width=500,height=500"); is(popup.innerHeight, 500); is(popup.innerWidth, 500); popup.resizeBy(50, 0); ... When resizeBy() is called, platform code nsGlobalWindow::CanMoveResizeWindows() in the content process needs to know if the window has siblings or not. But async messages sent from the parent tabbrowser.xml code when window.open() is called are not processed in the child until after resizeBy() is called. A window.open() might result in a new tab or a new window. I think I've addressed this by changing the CreatedWindowInfo IPDL struct to include a hasSiblings member so that when a new window is created, the parent passes back the correct hasSiblings value (whether a tab or new window is created) so subsequent resize checks can be correct. But I'm hitting an intermittent failure on Linux that needs to be resolved.
Keywords: perf
Whiteboard: [qf:p2] → [qf:p1]
Whiteboard: [qf:p1] → [qf:i60][qf:p1]
Are you going to be able to get back to this, Haik? If not, I'll try to find someone to take over your work here. Thanks.
Flags: needinfo?(haftandilian)
(In reply to Andrew Overholt [:overholt] from comment #56) > Are you going to be able to get back to this, Haik? If not, I'll try to find > someone to take over your work here. Thanks. Sorry, I don't think I'll be able to get back to this in the short term. If someone else can take over the work, that would be great. Thanks.
Flags: needinfo?(haftandilian) → needinfo?(overholt)
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]
Blake said he'd take this :)
Assignee: haftandilian → mrbkap
Flags: needinfo?(overholt)
Whiteboard: [qf:f60][qf:p1] → [qf:f61][qf:p1]
(In reply to Haik Aftandilian [:haik] from comment #55) > I think I've addressed this by changing the CreatedWindowInfo IPDL struct to > include a hasSiblings member so that when a new window is created, the > parent passes back the correct hasSiblings value (whether a tab or new > window is created) so subsequent resize checks can be correct. But I'm > hitting an intermittent failure on Linux that needs to be resolved. Can you update the review request in this bug so that I can avoid duplicating this work?
Flags: needinfo?(haftandilian)
Attachment #8865639 - Flags: review?(dao+bmo)
(In reply to Andrew Overholt [:overholt] from comment #58) > Blake said he'd take this :) Blake, thank you for agreeing to work on this. (In reply to Blake Kaplan (:mrbkap) from comment #59) > (In reply to Haik Aftandilian [:haik] from comment #55) > > I think I've addressed this by changing the CreatedWindowInfo IPDL struct to > > include a hasSiblings member so that when a new window is created, the > > parent passes back the correct hasSiblings value (whether a tab or new > > window is created) so subsequent resize checks can be correct. But I'm > > hitting an intermittent failure on Linux that needs to be resolved. > > Can you update the review request in this bug so that I can avoid > duplicating this work? It's posted. The patches were from back in October, descendant of changeset 384349:ee1c41cf306d: 384349:ee1c41cf306d Wed Oct 04 10:43:27 2017 -0700 ffxbld No bug, Automated blocklist update from host bld-linux64-spot-324 - a=blocklist-update browser/app/blocklist.xml In case it helps, I was running these three tests for doing quick validation tests locally. browser/components/sessionstore/test/browser_906076_lazy_tabs.js dom/tests/mochitest/bugs/test_resize_move_windows.html dom/tests/mochitest/general/test_resizeby.html
Flags: needinfo?(haftandilian)
Whiteboard: [qf:f61][qf:p1] → [qf:f64][qf:p1]
Sorry for the delay here. I've done several try runs on my end, as well as running the tests Haik mentioned in comment 63 and I haven't had any failures. I've also addressed Dao's comments in comment 54, so I'll resubmit this for review and hopefully get it checked in.
Whiteboard: [qf:f64][qf:p1] → [qf:p1:f64]
Dao, ping?
Comment on attachment 8983224 [details] Bug 1350642 - Keep the tabCountResizable property in sync; https://reviewboard.mozilla.org/r/249086/#review262186 ::: browser/base/content/tabbrowser.js:1713 (Diff revision 1) > evt.initEvent("TabRemotenessChange", true, false); > tab.dispatchEvent(evt); > > + tab.linkedBrowser > + .messageManager > + .sendAsyncMessage("Browser:HasSiblings", this.tabs.length > 1); nit: It doesn't matter in this case, but generally the TabRemotenessChange event should be dispatched last after all that other stuff ::: browser/base/content/tabbrowser.js:2454 (Diff revision 1) > } > throw e; > } > > - // Hack to ensure that the about:newtab, and about:welcome favicon is loaded > + // Update TabChild hasSiblings properties > + if (!aCreateLazyBrowser) { We need to do this too when a lazy browser later becomes non-lazy, right? In other words, shouldn't we do this _insertBrowser? ::: browser/base/content/tabbrowser.js:2475 (Diff revision 1) > + t.linkedBrowser > + .messageManager > + .sendAsyncMessage("Browser:HasSiblings", this.tabs.length > 1); > + } > + > + // Hack to ensure that the about:newtab favicon is loaded nit: keep the original comment mentioning about:welcome
Attachment #8983224 - Flags: review?(dao+bmo)
I'll needinfo Blake in case he didn't see Dao's comment.
Flags: needinfo?(mrbkap)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41c8532f4a43b2d7f30924b57370756bcdef99e3 I realized that the message sending Browser:HasSiblings to the initial browser was redundant since we default to false anyway. That let me ignore the fact that sometimes messageManager is undefined (I guess a window restored from session restore never adds its initial browser to the tabstrip and so never initializes the browser element). The original code only sent the message if the initial browser had a tab associated with it.
Bug 1350642 - Keep the tabCountResizable property in sync; r?dao
Hm, that didn't quite do what I wanted.
Attachment #8865639 - Attachment is obsolete: true
Attachment #8874642 - Attachment is obsolete: true
Attachment #8956562 - Attachment is obsolete: true
Attachment #8983223 - Attachment is obsolete: true
Attachment #8983224 - Attachment is obsolete: true
Comment on attachment 8992736 [details] Bug 1350642 - Remove the PBrowser::Msg_GetTabCount sync IPC; r=krizsa Andrew McCreight [:mccr8] has approved the revision. https://phabricator.services.mozilla.com/D2196
Attachment #8992736 - Flags: review+
Comment on attachment 8992738 [details] Bug 1350642 - Keep the tabCountResizable property in sync; r?dao Dão Gottwald [::dao] has approved the revision. https://phabricator.services.mozilla.com/D2197
Attachment #8992738 - Flags: review+
Not sure if there's a way to "r+ with these changes" in phabricator. Looks like I can only either r+ or request changes (i.e. cancel the review). I've r+'d this now but also made two more review comments regarding s/tab/browser/. phabricator now thinks this is ready to land but this isn't quite true.
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/873a75c873e5 Remove the PBrowser::Msg_GetTabCount sync IPC; r=mccr8
Pushed by mrbkap@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5d45c705980a Keep the tabCountResizable property in sync; r=dao
https://hg.mozilla.org/mozilla-central/rev/873a75c873e5 https://hg.mozilla.org/mozilla-central/rev/5d45c705980a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
Performance Impact: --- → P1
Whiteboard: [qf:p1:f64]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: