Closed Bug 1696718 Opened 4 years ago Closed 4 years ago

context menu in addon popup misplaced with fission

Categories

(Core :: Layout, defect, P3)

Unspecified
All
defect

Tracking

()

RESOLVED FIXED
88 Branch
Fission Milestone M7
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- disabled
firefox87 --- disabled
firefox88 --- fixed

People

(Reporter: tnikkel, Assigned: hiro)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

On macOS (not sure about other platforms). Enable fission (fission.autostart=true). Install this very simple addon https://bugzilla.mozilla.org/attachment.cgi?id=9207132 by downloading it, unziping it, navigating to about:debugging#/runtime/this-firefox click "Load Temporary Add-on" and pick the manifest.json from the addon. Click the new button in the toolbar it added. Right click on the popup somewhere. Move the window. Open the popup, right click.

The right click menu will open in the wrong place.

regression range
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8c8bbc19e7f90f621bc92f805db1fddcdad605b4&tochange=d8bd960f9b82ca8d9ef340ae488673b6f0c0669e

-> bug 1493208

Confirmed that setting apz.popups.enabled=false fixes the issue.

Works on Linux. It looks like WidgetToScreenOffset() seems to be wrong. I will check it.

Flags: needinfo?(hikezoe.birchill)
OS: Unspecified → macOS

I was thinking that positioning content menu is using the same machinery for drop down menu, but it seems not. (which means the code in question is where I haven't touched.)

Interesting, now I can reproduce this on my Linux.

OS: macOS → All

If I am correctly tracking down what's going on there, there seems to be a race condition we don't properly call BrowserChild::RecvUpdateDimensions for the OOP iframe's document (i.e. w3.org).

Set release status flags based on info from the regressing bug 1493208

Timothy or Hiro, can you reproducible this bug without Fission?

kmag says we've had similar problems in the past when we were calculating coordinates based on the widget of the window, not the popup.

Fission Milestone: --- → ?

(In reply to Chris Peterson [:cpeterson] from comment #6)

Timothy or Hiro, can you reproducible this bug without Fission?

No, only with fission.

I was able to repro this with Fission and the experience is pretty bad. The context menu is not only misplaced but also flickers at least once at a different place before being displayed at the final position. This can be very annoying. We should fix this before going to Beta.
Hiro, can you fix this for M7 i.e. Fx88 (by March 18)?

Fission Milestone: ? → M7

Yep, I am trying.

Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED

What I've tracked down so far;

  1. This is not an APZ issue, with disabling apz.popups.enabled the context menu looks being properly positioned but in fact the menu is for the top level content document in the popup window not for the OOP iframe's one, so being properly positioned means it just behaves as if there is no iframe
  2. There is a race condition in between calling nsFrameLoader::UpdatePositionAndSize for the iframe and for the top level document (popup.html), if UpadatePositionAndSize for the ifame gets called after the one for the popup, this issue doesn't happen
  3. When UpdatePositionAndSize for the iframe gets called before the popup one, GetWindowDimensions returns an empty rect, so I assume the popup window hasn't been sized yet at the moment
  4. Based on my assumption in 3, I am supposing we need to make sure we call UpdatePositionAndSize for all descendant OOP iframes in popup window when the window gets resized
Component: Panning and Zooming → Layout
Flags: needinfo?(hikezoe.birchill)
Attached patch A hacky fix to fix the issue on Linux (obsolete) (deleted) — Splinter Review

Unfortunately the issue on Mac seems to be totally different from the issue I've been tracking down on Linux. Attaching patch is a hacky fix for the Linux issue, it invokes requestUpdatePosition() when extension's popup window gets resized and it tweaks the original requestUpdatePosition function to descend down all descendant browsing context and call BrowserParent::UpdatePosition for each browsing context so that when the popup window gets resized, the change is propagated into all documents in OOP iframes so context menu on OOP iframe documents is properly positioned. BUT the patch didn't fix the issue on Mac at all.

On my linux box, the issue on Linux does just happen on local debug builds, I've never seen the issue on official nightly builds, so I was thinking that''s because of the race condition I've noticed, I believe that's true on Linux, but that's not the case on Mac.

I will debug on Mac tomorrow.

Ok, the issue on Mac looks also the same race condition where BrowserChild::mClientOffset for OOP iframe documents isn't properly updated.

To summarize what the problem is, what I've found;

  1. For context menu positioning (also for dropdown menu etc.), we use PuppetWidget::GetWindowPosition() and the function ends up using BrowserChild::mClientOffset, the offset is wrong in extension's popups
  2. In normal OOP iframe documents in the browser window the offset gets updated by calling nsFrameLoader::UpdatePositionAndSize() in nsSubDocumentFrame::ReflowFinished when the browser window gets resized, or by calling BrowserParent::Udpate() via a MozUpdateWindowPos event when the browser window moved
  3. But in this extension's popup case, ReflowFinished isn't invoked when the popup window gets resized because the "resize" is a result of reflows of the contents (that said, this doesn't matter in this issue's case)
  4. What happens here when an extension popup window is open, we create a temporal panel for popup having a browser and starts loading the content in the browser and then we create another browser attaching the correct position and swap the browsers
  5. So if the contents inside the popup window has already finished reflows any OOP iframe documents inside the popup will never know the correct position where they've been attached

A reliable way to fix this issue I've found so far is to notify the position update to all BrowserParents for OOP iframe documents in nsMenuPopupFrame::SetPopupPosition and update the position in each BrowserParent.

Kris, does this approach sound reasonable? Another option I can think of is to do the notification in BrowserParent::UpdateDimensions for the extension browser but I don't want to add something for popup specifically in the UpdateDimensions function.

CCing :mattwoodrow, he might have some other ideas.

FWFW, here is the change for the approach.

Flags: needinfo?(kmaglione+bmo)

I finished writing a browser mochitest for this issue (the test is testing select drop down popup menu position instead of context menu position though), so I sent a review request to kmag.

Flags: needinfo?(kmaglione+bmo)
Attachment #9208321 - Attachment is obsolete: true
Severity: -- → S3
Priority: -- → P3
Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/01d98e7b02c8 Propagate popup window position update to descendants documents. r=kmag
Regressions: 1699079

Looks like the test is very flaky on TSAN builds, in M-fis jobs it fails with the SpecialPowers error, and interestingly it fails without Fission in this case the SpecialPowers error doesn't happen, it just fails what the test tries to test, I mean the popup window position is simply wrong. So for the non fission failure case, there must be at least one pre-existing issue. And for the SpecialPowers' case there are a bunch of intermittent failure bugs caused by the same error (bug 1674543 is one of them, for example). So I am going to skip the test on TSAN builds for now.

Flags: needinfo?(hikezoe.birchill)

Filed bug 1699341 to enable the test.

Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7acd0f6ce285 Propagate popup window position update to descendants documents. r=kmag
Regressions: 1699348
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
QA Whiteboard: [qa-88b-p2]
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: