nsChildView::GetEditorView can flush layout at a bad time.
Categories
(Core :: Widget: Cocoa, defect, P3)
Tracking
()
People
(Reporter: emilio, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
(Whiteboard: widget-next)
Attachments
(1 file)
|
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
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.
|
||
Updated•6 years ago
|
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)?
|
Assignee | |
Comment 2•6 years ago
|
||
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?
|
|
Assignee | |
Comment 3•6 years ago
|
||
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.
|
||
Updated•6 years ago
|
browser_library_middleclick.js should be re-enabled should be reverted as part of this fix.
|
|
Assignee | |
Comment 5•6 years ago
|
||
|
|
Assignee | |
Comment 6•6 years ago
|
||
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.
|
||
Comment 8•6 years ago
|
||
| bugherder | ||
|
||
Comment 9•6 years ago
|
||
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
|
Reporter | |
Comment 10•6 years ago
|
||
(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.
|
|
||
Comment 11•6 years ago
|
||
(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.
|
|
Reporter | |
Comment 12•6 years ago
|
||
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?
|
|
Reporter | |
Comment 14•6 years ago
|
||
Potentially, yeah... In practice the assert was never there :(
|
|
Assignee | |
Comment 15•6 years ago
|
||
Sure, the patch should be safe to uplift. I'll request it after verifying whether it can graft cleanly.
|
|
Assignee | |
Comment 16•6 years ago
|
||
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
|
|
||
Comment 17•6 years ago
|
||
Comment on attachment 9055416 [details]
Bug 1530188 - Make nsChildView::GetEditorView() use eQueryContentState without flushing layout
uplift accepted for 67 beta 11, thanks.
|
||
Comment 18•6 years ago
|
||
| bugherder uplift | ||
Description
•