Closed Bug 1267844 Opened 4 years ago Closed 4 years ago

Infinite throbber shown when moving tab to another window

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
e10s m9+ ---
firefox48 + verified
firefox49 --- verified

People

(Reporter: marco, Assigned: mrbkap)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached video screencast.webm
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).
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)
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)
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)
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).
(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.
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).
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)
tracking-e10s: --- → ?
OS: Linux → Unspecified
Hardware: x86_64 → Unspecified
Assignee: nobody → mrbkap
Component: Untriaged → Tabbed Browser
Attached patch Patch v1Splinter Review
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)
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?
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+
Flags: needinfo?(mrbkap)
https://hg.mozilla.org/mozilla-central/rev/351a97722d68
https://hg.mozilla.org/mozilla-central/rev/2edf2e84ac13
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Duplicate of this bug: 1274163
Component: Tabbed Browser → IPC
Product: Firefox → Core
Target Milestone: Firefox 49 → ---
Target Milestone: --- → mozilla49
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 )
Flags: qe-verify+
Flags: needinfo?(mrbkap)
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)
Status: RESOLVED → VERIFIED
Flags: needinfo?(mcastelluccio)
Blocks: 1254865
You need to log in before you can comment on or make changes to this bug.