Closed
Bug 1267844
Opened 8 years ago
Closed 8 years ago
Infinite throbber shown when moving tab to another window
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
VERIFIED
FIXED
mozilla49
People
(Reporter: marco, Assigned: mrbkap)
References
Details
(Keywords: regression)
Attachments
(2 files)
6.00 MB,
video/webm
|
Details | |
8.36 KB,
patch
|
mconley
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
When I move a tab from a window to another, a throbber is shown (indefinitely) instead of the actual page. When I move the tab out of the window, the tab is instantly loaded. I'm using Nightly (49).
Comment 1•8 years ago
|
||
This sounds very similar to bug 1264402. We're having no luck reproducing that one reliably. Are you able to reproduce this reliably? And with a new profile?
Flags: needinfo?(mcastelluccio)
Reporter | ||
Comment 2•8 years ago
|
||
At the moment, I can reproduce it reliably in my profile. Looks like I can reproduce it only if I move the tab in a window that has enough tabs to show the arrow buttons on the sides of the tab bar.
Flags: needinfo?(mcastelluccio)
Comment 3•8 years ago
|
||
Hey mcote - this sounds a lot like your bug (bug 1264402)... marco is only able to reproduce when the tabstrip is in the "overflowed" state. Perhaps that's the key to this - are you able to reproduce if you get into that same state?
Flags: needinfo?(mcote)
Reporter | ||
Comment 4•8 years ago
|
||
I'm also able to reproduce in a clean profile, I'm using mozregression to find a regression range (for now 2016-01-01 is good).
Comment 5•8 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #4) > I'm also able to reproduce in a clean profile, I'm using mozregression to > find a regression range (for now 2016-01-01 is good). Excellent, thank you.
Keywords: regressionwindow-wanted
Reporter | ||
Comment 6•8 years ago
|
||
mozregression points me to bug 1254865. This is the same conclusion as bug 1264402, so they could be duplicates (although this bug has nothing to do with multiple displays).
Reporter | ||
Updated•8 years ago
|
status-firefox48:
--- → affected
Keywords: regressionwindow-wanted → regression
Comment 7•8 years ago
|
||
Hey, yeah! That would explain why it wasn't reproducing at first; I had cleaned out tonnes of tabs. Indeed, even if I open a bunch of blank tabs, as soon as there are too many to display at once, I always get the loading throbber after dragging. Then I close enough so that they're all displayed, and the page loads fine after dragging. Yay, fully reproducible!
Flags: needinfo?(mcote)
Updated•8 years ago
|
Updated•8 years ago
|
Assignee: nobody → mrbkap
Updated•8 years ago
|
Component: Untriaged → Tabbed Browser
Assignee | ||
Comment 9•8 years ago
|
||
We were failing to fire the MozLayerTreeReady event. What's happening is this: * tabbrowser creates a new tab for the drop (and swap) * it sets this.selectedTab to be the new tab * it does the docshell swap * it closes the old tab However, in the middle of the swap, we receive a (shadow) layer update for the tab off the main thread, causing us to fire the internal observer. This creates a Runnable, posted to the main thread, which fires the MozLayerTreeReady for the TabParent associated with its mLayersId. However, the swap changes the layers ID! Therefore, by the time we run the runnable, we've closed the old TabParent and removed the layers ID that the runnable was created with from the layers ID -> TabParent table, dropping the notification on the floor. My patch fixes this by holding onto the internal listener object and swapping it and its internal state to match its TabParent's. This means that even though we posted the runnable before the swap, when it runs, it can still get its hands on the right TabParent. I decided to hold a (weak) pointer to the tab parent and clear it during destruction instead of the layers ID. I could go back to holding onto the layers ID, but that seems like another (unnecessary) layer of indirection on top. Mike, what do you think?
Attachment #8751501 -
Flags: review?(mconley)
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=717298ba0cf0
Comment 11•8 years ago
|
||
Comment on attachment 8751501 [details] [diff] [review] Patch v1 Review of attachment 8751501 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay here, mrbkap. This looks mostly good - just have a question about the swap. ::: dom/ipc/TabParent.cpp @@ +2951,5 @@ > + > + // No need for a lock, destruction can only happen on the main thread and we > + // only read mLayerUpdateObserver::mTabParent on the main thread. > + Swap(mLayerUpdateObserver, aOther->mLayerUpdateObserver); > + mLayerUpdateObserver->Swap(aOther->mLayerUpdateObserver); Hold on - am I reading this right? We swap this->mLayerUpdateObserver with aOther->mLayerUpdateObserver... and then swap them back?
Updated•8 years ago
|
Flags: needinfo?(mrbkap)
Comment 12•8 years ago
|
||
Comment on attachment 8751501 [details] [diff] [review] Patch v1 Repeating our conversation in IRC: 1) I'm cool with holding weakly onto TabParent, and having it be nulled out by TabParent::DestroyInternal mrbkap says that using a traditional WeakPtr would have complications, and I trust him. 2) Let's rename LayerTreeUpdateObserver::Swap to something more precise, like LayerTreeUpdateObserver::SwapTabParent.
Attachment #8751501 -
Flags: review?(mconley) → review+
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mrbkap)
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/351a97722d68 https://hg.mozilla.org/mozilla-central/rev/2edf2e84ac13
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•8 years ago
|
Component: Tabbed Browser → IPC
Product: Firefox → Core
Target Milestone: Firefox 49 → ---
Updated•8 years ago
|
Target Milestone: --- → mozilla49
Comment 17•8 years ago
|
||
Want to request uplift to 48 aurora? We could also verify the fix on nightly (but I'm ok with uplifting, then verifying on aurora as long as that happens this week )
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8751501 [details] [diff] [review] Patch v1 Approval Request Comment [Feature/regressing bug #]: n/a [User impact if declined]: Dragging tabs between windows could fail to paint in the new window. [Describe test coverage new/current, TreeHerder]: Low test coverage. [Risks and why]: Low-medium risk -- there are multiple threads involved, which automatically increases the risk of any patches. However, I believe that this fixes the race and cannot introduce new memory races. I would trust this patch. [String/UUID change made/needed]: n/a
Flags: needinfo?(mrbkap)
Attachment #8751501 -
Flags: approval-mozilla-aurora?
Comment on attachment 8751501 [details] [diff] [review] Patch v1 Improves dragging tabs across windows in e10s, Aurora48+
Attachment #8751501 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Marco, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(mcastelluccio)
Comment 21•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/c36af26156fa
Reporter | ||
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Flags: needinfo?(mcastelluccio)
Comment 22•8 years ago
|
||
Verified fixed FX 48b1 Win 7
You need to log in
before you can comment on or make changes to this bug.
Description
•