nsChildView::GetEditorView can flush layout at a bad time.

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: emilio, Assigned: masayuki)

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed, firefox68 fixed)

Details

(Whiteboard: widget-next)

Attachments

(1 attachment)

See https://crash-stats.mozilla.com/report/index/512ce92e-022b-4672-8680-1e9580190224 for example.

Flushing layout there is not sound, and can cause security issues.

See Also: → 1530190
Priority: -- → P3
Whiteboard: widget-next

nsChildView's windowLevel Cocoa method ends up dispatching a ContentQueryEvent in order to find an editor view to get a windowLevel out of that. That code is from many, many OS X releases ago (predates Firefox 4).

Would it now be possible to implement windowLevel in a way that does not dispatch an event and, therefore, doesn't flush layout?

Can we prioritize this higher than P3 considering that this bug is interfering with Fission IME enablement (bug 1524975 comment 35)?

Flags: needinfo?(mstange)
Flags: needinfo?(masayuki)

I think that we don't need to flush layout in this case because layout changes in the panel does not cause changing the window level. Should I take this?

Flags: needinfo?(masayuki)

Ah, or, we should return actual window level when it's a popup window because it's a hack for IME. IME always works with active view. Therefore, we need this hack only for the case.

Okay, this must require to test with a lot of IMEs. I should take this.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)

browser_library_middleclick.js should be re-enabled should be reverted as part of this fix.

nsChildView::GetEditorView() is called by TextInputHandlerBase::GetWindowLevel()
which is called when Cocoa requests window level of focused widget.

It currently gets widget including focused element (e.g., it may be in a XUL
<panel>) with eQueryTextContent event. However, it requires only the widget
(i.e., when a XUL <panel> has focused element, the widget for the panel).
Therefore, it does not require to flush the layout.

However, on macOS, ContentEventHandler always flushes layout even with
eQueryContentState which does not require any layout information. Whether
it requires flushing layout or not is considered with
WidgetQueryContentEvent::mNeedsToFlushLayout but this is set to false only
when IMEContentObserver notifies widget (and IME) of focus set. At this
time, only on macOS, IME caches the layout information, for example, the
character coordinates, but we don't have a way to update it. This is the reason
why we always flush layout on macOS.

Unfortunately, when a menu popup frame is created, widget for the popup is
created synchronously. Then, Cocoa retrieves window level of the widget including
focused element. But this is unsafe to flush the layout. So, we need to stop
flushing layout in this case.

Therefore, this patch moves the #ifdef from TextEvents.h to
IMEContentObserver.cpp, then, makes nsChildView::GetEditorView() use
eQueryContentState which is the simplest query content event, and finally,
sets mNeedsToFlushLayout to false.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/edbde6116eee
Make nsChildView::GetEditorView() use eQueryContentState without flushing layout r=smaug
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Masayuki, would uplifting this patch to beta improve our crash rate in bug 1530177? Is that a patch that you think would be safe to uplift to beta? Thanks

Flags: needinfo?(masayuki)

(In reply to Pascal Chevrel:pascalc from comment #9)

Masayuki, would uplifting this patch to beta improve our crash rate in bug 1530177? Is that a patch that you think would be safe to uplift to beta? Thanks

Note that bug 1530177 will crash on early beta and nightly but not release.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

(In reply to Pascal Chevrel:pascalc from comment #9)

Masayuki, would uplifting this patch to beta improve our crash rate in bug 1530177? Is that a patch that you think would be safe to uplift to beta? Thanks

Note that bug 1530177 will crash on early beta and nightly but not release.

Do you mean that the crashes only happen behind the EARLY_BETA_OR_EARLIER? Because I see crashes in beta 9 and that flag was unset after beta 8.

They only happen when MOZ_DIAGNOSTIC_ASSERT_ENABLED is defined. Looking at the crashes in b9, one of them (the Mac one) is related to this bug and is crashing on that assertion:

https://crash-stats.mozilla.com/report/index/f892519c-4309-492f-a9e1-013460190410

So it seems like the diagnostic assertion is enabled there.

When the diagnostic assert doesn't take effect, the consequences are potentially worse, aren't they?

Potentially, yeah... In practice the assert was never there :(

Sure, the patch should be safe to uplift. I'll request it after verifying whether it can graft cleanly.

Flags: needinfo?(masayuki)

Comment on attachment 9055416 [details]
Bug 1530188 - Make nsChildView::GetEditorView() use eQueryContentState without flushing layout

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Not sure whether this is regression
  • User impact if declined: May have security issue due to bad time reflow.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just changing to simplest query event for avoiding flushing pending reflow (the event was not when I implemented it, so, using the old event was a hack).
  • String changes made/needed: none
Attachment #9055416 - Flags: approval-mozilla-beta?

Comment on attachment 9055416 [details]
Bug 1530188 - Make nsChildView::GetEditorView() use eQueryContentState without flushing layout

uplift accepted for 67 beta 11, thanks.

Attachment #9055416 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
See Also: → 1551604
You need to log in before you can comment on or make changes to this bug.