Closed Bug 1428839 Opened 6 years ago Closed 6 years ago

Make popuphidden handling in PanelMultiView synchronous and prevent re-entrancy

Categories

(Firefox :: Toolbars and Customization, enhancement, P1)

Product:
Firefox
Component:
Toolbars and Customization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: sfoster, Assigned: Paolo)

References

Details

(Whiteboard: [reserve-photon-structure] )

Attachments

(8 files, 5 obsolete files)

Bug 1428839 - Part 1 - Remove previous workaround for flickering at the end of the transition.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1428839 - Part 2 - Add a function to determine if a view is still open.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1428839 - Part 3 - Clean up view properties when opening them.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1428839 - Part 4 - Avoid re-entrancy in PanelMultiView navigation functions.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1428839 - Part 5 - Open the anchor when subview navigation starts and close it asynchronously afterwards.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1428839 - Part 6 - Remove previous workaround for the panel resizing at the end of the transition.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1428839 - Part 7 - Reduce calls to _cleanupTransitionPhase.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1428839 - Part 8 - Fix the sliding transition when views are reordered.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
There is currently a potential race when a PMV closes, as it awaits reflows while resetting style and attributes and making the main view current. While some of that work is arguably optional, the current state of things is prone to intermittent test failures and a bit brittle. 

Having a sync way to reset the panel back to the initial state without going down the necessarily-async showSubView path would be a big improvement.
Component: Theme → Toolbars and Customization
My hunch is that having the popuphidden handler call showMainView/showSubView with all the async steps that involves is the root of the test intermittents I'm seeing from my patch in bug 1402845. While that's mostly a test problem and only a very unlikely edge case for real users, this still seems like a design flaw to me: teardown should happen in a single tick whenever possible. 

:paolo, is this something you might have time to tackle? (and do you agree with the diagnosis?).
Flags: needinfo?(paolo.mozmail)
I agree, this is basically what I filed as bug 1416498, although I left the solution more open to interpretation.

In fact my guess is that we don't really need to do any work on teardown, we can likely set up everything when the panel is opening. We probably also have to ensure that any other currently displayed panel is hidden before a new one is opened programmatically, either by closing it or by refusing to open the other panel.

We also need to prevent re-entering the functions that set up subviews. Since subviews can be reused as main views, this may mean that we have to make the panel first opening asynchronous, which is quite feasible because all the panels are opened with JavaScript calls, although there may be some tests to update which emulate clicks and check panel contents immediately.

I may be able to look into this as it also blocks bug 1420939.
Flags: needinfo?(paolo.mozmail)
As I expected, there is quite some work to do to make these race conditions go away. The complexity of the code and the number of small hacks has increased while we worked on Photon, due to the need to focus on individual fixes rather than a more general plan. For example, the whole concept of ephemeral panels and borrowing views was built by necessity while keeping compatibility with the non-Photon model.

I started to work on this missing general plan in this bug. The model I'm proposing involves making the least work possible when the panel is hidden, preparing the views before showing them instead. This will allow us to do less or no work on destruction, for example when closing windows.

The transition code should also be ready to cease asynchronous work immediately when a different panel borrows a view from the current panel. Cleaning up will not be necessary if the other panel takes care of re-initializing all the display attributes and dimensions properly.

While not all the steps to get there are trivial, it can be done gradually, and the upside is that I already found some visible race conditions that happen in production when views are reused, and these will likely be gone at the end of this work.

I have an initial set of patches and more are coming, this will land after the soft code freeze anyways.
Mike, I made review requests to Gijs as you're still in triage mode, but feel free to steal them or add any comment.
Comment on attachment 8943304 [details]
Bug 1428839 - Part 1 - Remove the setMainView methods.

https://reviewboard.mozilla.org/r/213612/#review219696
Attachment #8943304 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8943305 [details]
Bug 1428839 - Part 2 - Ensure that views passed to showSubView are added to the list of known views without having to reset the internal _panelViews variable.

https://reviewboard.mozilla.org/r/213614/#review219702
Attachment #8943305 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8943306 [details]
Bug 1428839 - Part 3 - Separate the set of known views from the stack of open views.

https://reviewboard.mozilla.org/r/213616/#review219708

::: browser/components/customizableui/PanelMultiView.jsm:11
(Diff revision 1)
> + * declared using <panelview> elements that are usually children of the main
> + * <panelmultiview> element, although they don't need to, as views can be

"need to be"

::: browser/components/customizableui/PanelMultiView.jsm:13
(Diff revision 1)
> + * other subviews in a cascade menu pattern.
> + *
> + * The <panel> element should contain a <panelmultiview> element. Views are
> + * declared using <panelview> elements that are usually children of the main
> + * <panelmultiview> element, although they don't need to, as views can be
> + * borrowed from other panels or popup sets.

"borrow" suggests we put them back in the same place we took them from. I don't think we do. :-)

Maybe "... views can also be used ..." ?

::: browser/components/customizableui/PanelMultiView.jsm:38
(Diff revision 1)
> + *         └───┴───┴───┴───┘        └───┘
> + *               │
> + *               └── Currently visible view
> + *
> + * If the <panelmultiview> element is "ephemeral", borrowed subviews will be
> + * moved out again so that the panel element can be removed safely.

Maybe say *where* they are moved to.

::: browser/components/customizableui/PanelMultiView.jsm:172
(Diff revision 1)
>      if (testMode)
>        return;
>  
> -    this._currentSubView = this._subViewObserver = null;
> +    this.knownViews = new Set(this.node.getElementsByTagName("panelview"));
> +    this.openViews = [];
> +    this._subViewObserver = null;

As far as I can see, `_subViewObserver` is unused. Please remove this line.

::: browser/components/customizableui/PanelMultiView.jsm:330
(Diff revision 1)
>    _canGoBack(view = this._currentSubView) {
>      return view.id != this._mainViewId;
>    }
>  
>    showMainView() {
> -    if (!this._mainViewId)
> +    if (!this.node || !this._mainViewId)

When does this (`!this.node`) happen? And why do we need to catch it here?

::: browser/components/customizableui/PanelMultiView.jsm:343
(Diff revision 1)
>     * are hidden, except one specifically.
>     *
>     * @param {panelview} [theOne] The panelview DOM node to ensure is visible.
>     *                             Optional.
>     */
>    hideAllViewsExcept(theOne = null) {

Is it possible for this to be called with a `theOne` that isn't in `knownViews` ?
Attachment #8943306 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8943306 [details]
Bug 1428839 - Part 3 - Separate the set of known views from the stack of open views.

https://reviewboard.mozilla.org/r/213616/#review219708

> When does this (`!this.node`) happen? And why do we need to catch it here?

showMainView is a public method and there is at least one case where it's called after destruction, making _mainViewId trip.

In practice the showMainView method is not necessary at all, as we declare which view to open first using the attribute, and it can't be used to navigate back anyways. It will probably disappear later in the patch series.

> Is it possible for this to be called with a `theOne` that isn't in `knownViews` ?

I don't think so, by looking at the call sites.
Comment on attachment 8943664 [details]
Bug 1428839 - Part 5 - Move more methods and properties to the PanelView class.

https://reviewboard.mozilla.org/r/214048/#review219752

::: browser/components/customizableui/PanelMultiView.jsm:115
(Diff revision 1)
> +   * Helper method to emit an event on an element, whilst also making sure that
> +   * the correct method is called on CustomizableWidget instances.

"Dispatches a custom event on this element."
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Depends on: 1432015
Depends on: 1432016
Attachment #8943304 - Attachment is obsolete: true
Attachment #8943305 - Attachment is obsolete: true
Attachment #8943306 - Attachment is obsolete: true
Attachment #8943663 - Attachment is obsolete: true
Attachment #8943663 - Flags: review?(gijskruitbosch+bugs)
Attachment #8943664 - Attachment is obsolete: true
Attachment #8943664 - Flags: review?(gijskruitbosch+bugs)
No longer depends on: 1432016
Depends on: 1424264
Depends on: 1434883
Depends on: 1439358
Depends on: 1441284
Priority: -- → P1
Summary: Make popuphidden handling in PanelMultiView synchronous → Make popuphidden handling in PanelMultiView synchronous and prevent re-entrancy
Blocks: 1420939
Comment on attachment 8954410 [details]
Bug 1428839 - Part 2 - Add a function to determine if a view is still open.

https://reviewboard.mozilla.org/r/223498/#review229854
Attachment #8954410 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954409 [details]
Bug 1428839 - Part 1 - Remove previous workaround for flickering at the end of the transition.

https://reviewboard.mozilla.org/r/223496/#review229912
Attachment #8954409 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954411 [details]
Bug 1428839 - Part 3 - Clean up view properties when opening them.

https://reviewboard.mozilla.org/r/223500/#review229960

::: browser/components/customizableui/PanelMultiView.jsm:738
(Diff revision 3)
>      }
>  
> +    // Clean up all the attributes and styles related to transitions. We do this
> +    // here rather than when the view is closed because we are likely to make
> +    // other DOM modifications soon, which isn't the case when closing.
> +    let style = panelView.node.style;

Nit: deconstructuring assignment here would make this shorter.
Attachment #8954411 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954412 [details]
Bug 1428839 - Part 4 - Avoid re-entrancy in PanelMultiView navigation functions.

https://reviewboard.mozilla.org/r/223502/#review229962
Attachment #8954412 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954413 [details]
Bug 1428839 - Part 5 - Open the anchor when subview navigation starts and close it asynchronously afterwards.

https://reviewboard.mozilla.org/r/223504/#review229964
Attachment #8954413 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954414 [details]
Bug 1428839 - Part 6 - Remove previous workaround for the panel resizing at the end of the transition.

https://reviewboard.mozilla.org/r/223506/#review229978
Attachment #8954414 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8954590 [details]
Bug 1428839 - Part 7 - Reduce calls to _cleanupTransitionPhase.

https://reviewboard.mozilla.org/r/223684/#review229984
Attachment #8954590 - Flags: review?(gijskruitbosch+bugs) → review+
Can you clarify how I can reproduce the issue part 8 is supposed to fix? You tried to tell me over vidyo but I didn't reproduce then, and by now I'm confused over the sequence of events and still can't reproduce.
Flags: needinfo?(paolo.mozmail)
Can you reproduce it with the following 7 clicks right after opening the browser window?

1. Main menu button > Library > Bookmarks
2. Library button on the toolbar
3. Repeat step 1
Flags: needinfo?(paolo.mozmail) → needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #47)
> Can you reproduce it with the following 7 clicks right after opening the
> browser window?
> 
> 1. Main menu button > Library > Bookmarks
> 2. Library button on the toolbar
> 3. Repeat step 1

Yep, that helps, thanks.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8954591 [details]
Bug 1428839 - Part 8 - Fix the sliding transition when views are reordered.

https://reviewboard.mozilla.org/r/223686/#review230048

::: browser/components/customizableui/PanelMultiView.jsm:901
(Diff revision 2)
> -      try {
> -        this._viewStack.insertBefore(viewNode, oldSibling);
> +      // Place back the view after all the other views that are already open in
> +      // order for the transition to work as expected.

For my edification, can you explain how exactly (ie "first this happens, then this happens, then this other thing happens") this caused the buggy behaviour we had before? Like, it "vaguely" makes sense that changing the order of the appends here can fix something like this, and I can verify visually that it is now fixed, but every time I'm trying to reason about why this would happen I am confused. I thought we appended the view node well before we started showing the subview here? And even if we didn't, how is the 'next sibling' here wrong?
Attachment #8954591 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f579f320031c
Part 1 - Remove previous workaround for flickering at the end of the transition. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/0605579057e6
Part 2 - Add a function to determine if a view is still open. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/288b230b356c
Part 3 - Clean up view properties when opening them. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/96dd6c0d53b9
Part 4 - Avoid re-entrancy in PanelMultiView navigation functions. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c4d69669156
Part 5 - Open the anchor when subview navigation starts and close it asynchronously afterwards. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca146d063f1
Part 6 - Remove previous workaround for the panel resizing at the end of the transition. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/39605c4715d1
Part 7 - Reduce calls to _cleanupTransitionPhase. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/724aad27fd44
Part 8 - Fix the sliding transition when views are reordered. r=Gijs
(In reply to :Gijs from comment #50)
> I thought we appended the view node well before we started showing the subview here?

Yes, but we don't move it if it's already a child of the view stack.

> And even if we didn't, how is the 'next sibling' here wrong?

1. Main menu button > Library > Bookmarks

The main panel contains the menu, Library, and Bookmarks views in order, as well as other views that don't count as they're hidden.

2. Library button on the toolbar

The main panel now contains only the menu and Bookmarks views in order.

3. Main menu button > Library

The main panel now contains the menu view, hidden Bookmarks view, and Library view in order.

4. Bookmarks

The issue appears since the Bookmarks view is now visible but is not moved to the last position.

Hope this helps!
Blocks: 1442187
Blocks: 1413159
No longer blocks: 1413159
Blocks: 1411919
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: