1342552 - Crash in nsViewManager::GetRootWidget

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-fc606b00-f700-442e-af4c-b9aa62170224.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsViewManager::GetRootWidget(nsIWidget**) 	view/nsViewManager.cpp:1072
1 	xul.dll 	nsPresContext::GetRootWidget() 	layout/base/nsPresContext.cpp:1116
2 	xul.dll 	mozilla::IMEStateManager::OnChangeFocusInternal(nsPresContext*, nsIContent*, mozilla::widget::InputContextAction) 	dom/events/IMEStateManager.cpp:403
3 	xul.dll 	mozilla::IMEStateManager::OnChangeFocus(nsPresContext*, nsIContent*, mozilla::widget::InputContextAction::Cause) 	dom/events/IMEStateManager.cpp:375
4 	xul.dll 	nsFocusManager::Focus(nsPIDOMWindowOuter*, nsIContent*, unsigned int, bool, bool, bool, bool, nsIContent*) 	dom/base/nsFocusManager.cpp:1958
5 	xul.dll 	nsFocusManager::WindowShown(mozIDOMWindowProxy*, bool) 	dom/base/nsFocusManager.cpp:910
6 	xul.dll 	nsGlobalWindow::SetReadyForFocus() 	dom/base/nsGlobalWindow.cpp:10164
7 	xul.dll 	nsGlobalWindow::SetReadyForFocus() 	dom/base/nsGlobalWindow.cpp:10157
8 	xul.dll 	PresShell::UnsuppressAndInvalidate() 	layout/base/nsPresShell.cpp:3820
9 	xul.dll 	PresShell::sPaintSuppressionCallback(nsITimer*, void*) 	layout/base/nsPresShell.cpp:1808
10 	xul.dll 	nsTimerImpl::Fire() 	xpcom/threads/nsTimerImpl.cpp:521
11 	xul.dll 	nsTimerEvent::Run() 	xpcom/threads/TimerThread.cpp:286
12 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1067
13 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:96
14 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
15 	xul.dll 	_SEH_epilog4

this cross-platform crash signature has been around for a while. many user comments indicate they were printing something at the time of the crash or hitting the back button to navigate to a prior site.

Correlations for Firefox Beta
(77.42% in signature vs 01.61% overall) Module "prnfldr.dll" = true [100.0% vs 02.32% if platform_pretty_version = Windows 7]
(96.77% in signature vs 28.84% overall) Module "winspool.drv" = true [100.0% vs 14.54% if platform_version = 6.1.7600]
(90.32% in signature vs 00.12% overall) address = 0xffffffffe5e5e5f1
(96.77% in signature vs 33.31% overall) Module "comdlg32.dll" = true [100.0% vs 18.48% if platform_version = 6.1.7600]
(100.0% in signature vs 33.85% overall) reason = EXCEPTION_ACCESS_VIOLATION_READ
(98.39% in signature vs 31.91% overall) top(none)/detached > 0 = null

(marking this as security sensitive due to the uaf situation)

Comment 1

[Daniel Veditz [:dveditz]]

Can we tell if there's a correlation in which IME people are using? or is it all in our code and independent of that?

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't think that this is related to IME.

Looks like that before firing the timer, the relation between PresShell, ViewManager and nsIWidget has been broken.

# FYI: If it's crashed in content process, we cannot know which IME is used at crash due to not loaded any DLL of IME in content process.

Comment 3

[Makoto Kato [:m_kato]]

This is during shutting down, so prescontext or presshell is already destroyed?

Comment 4

[Makoto Kato [:m_kato]]

ah, if not shutting down, But WindowShown is called, but nsViewManager is invalid. why...

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

When nsView is destroyed, it removes itself from nsViewManager:
https://dxr.mozilla.org/mozilla-central/rev/d29f84406483c721a13cf9a52936ecced0c5c98a/view/nsView.cpp#80,84,94,96-97

nsView::mViewManager is set by its constructor and never set to nullptr until destructor runs. So, the view can be root view after desctructor of nsView is run? The destructor is called from nsView::Destroy():
https://dxr.mozilla.org/mozilla-central/rev/d29f84406483c721a13cf9a52936ecced0c5c98a/view/nsView.cpp#184,186

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsViewManager::SetRootView() is called by PresShell, nsDocumentView and nsPrintEngine:
https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsViewManager%3A%3ASetRootView%28nsView+%2A%29%22

The former 2 call it with the result of nsViewManager::CreateView().  On the other hand, nsPrintEngine may not do so in some cases.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If we could stop handling focus with detecting the destroy, perhaps, it should be stopped in nsFocusManager.

Comment 8

[Andrew Overholt [:overholt]]

(In reply to Masayuki Nakano [:masayuki] from comment #7)
> If we could stop handling focus with detecting the destroy, perhaps, it
> should be stopped in nsFocusManager.

Is that something you could handle or would you like me to try to find someone else to take it?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that it's better somebody who is really familiar with the relation between view and those 3 classes to take this bug. I cannot say "must be fixed with this patch" without STR...

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Many reports which have comment claim that it occurs during printing:
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=nsViewManager%3A%3AGetRootWidget&date=%3E%3D2017-01-01T07%3A10%3A00.000Z&date=%3C2017-04-04T07%3A10%3A00.000Z#comments

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, looks like that nsPrintEngine has a bug. nsPrintEngine::SetRootView() also crashes at setting root view:
https://crash-stats.mozilla.com/report/index/45e8c9a4-eb2a-472b-a88e-be90d2170227
> 0 	xul.dll 	nsPrintEngine::SetRootView(nsPrintObject*, bool&, bool&, nsSize&) 	layout/printing/nsPrintEngine.cpp:2102
> 1 	xul.dll 	nsPrintEngine::ReflowPrintObject(nsPrintObject*) 	layout/printing/nsPrintEngine.cpp:2165
> 2 	xul.dll 	nsPrintEngine::ReflowDocList(nsPrintObject*, bool) 	layout/printing/nsPrintEngine.cpp:1798

https://hg.mozilla.org/releases/mozilla-release/annotate/327e081221b0/layout/printing/nsPrintEngine.cpp#l2102

Comment 13

[Andrew Overholt [:overholt]]

Olli, can you please take a look here?

Comment 14

[Julien Cristau [:jcristau] (back April 22)]

Too late for firefox 52, mass-wontfix.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Although, I'm still not sure the actual reason of using released nsView.

If PresShell or DocShell is being destroyed properly in such case, my experimental patch could "hide" this bug.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e7b9149c5c16ce80f046efb0640afb19d5ad44f

I'm not sure if I should file another bug as just a cleanup code bug or something for it.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

My random thoughts:

* nsView isn't refcountable, it's destroyed when its owner frame is destroyed, it's a root view of an nsViewManager and it's being destroyed, or its parent whose nsViewManager is same and is being destroyed.
* So, nsView pointer can be dirty easily.  If it's possible, it should be managed with |UniquePtr<nsView>| and referenced by |const UniquePtr<nsView>&|.
* It might be possible nsViewManager to store outdated nsView pointer as its root if SetRootView() is called with outdated pointer.
* Non-fresh nsView instance is set only from nsPrintEngine::SetRootView(): http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/printing/nsPrintEngine.cpp#2084,2086,2102
* The parent view might be outdated: http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/printing/nsPrintEngine.cpp#2081,2089,2093
* nsPrintEngine::GetParentViewForRoot() returns the result of nsIContentViewer::FindContainerViewer(): http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/printing/nsPrintEngine.cpp#2024,2026-2027,2029
* nsIContentViewer::FindContainerViewer() returns a child view of its view: http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/base/nsDocumentViewer.cpp#2522,2557 http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/generic/nsSubDocumentFrame.cpp#1206,1208-1209,1213,1218,1223,1226
* However, nsSubDocumentFrame::mInnerView isn't cleared when its parent (mOuterView) is destroyed. On the other hand, it's a frame class, so, when mOuterView is destroyed, the instance should also be deleted...
* On the other hand, nsPrintEngine::ReconstructAndReflow() is scared. It refers cached nsPrintObject in for loop and also flushes pending notifications! http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/layout/printing/nsPrintEngine.cpp#1589-1590,1614,1623
* So, mPrintDocList should be nsTArray<const UniquePtr<nsPrintObject>&>, instead of nsTArray<nsPrintObject*>.


FYI: I tried to catch the case when nsPrintEngine::SetRootView() reuses the root view but PresShell has gone yesterday. But I couldn't be hit it...

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Focus handler doesn't need to try to move focus if new focused document is being destroyed

I'm not sure if this is possible case. Enn, how about you?

Comment 18

[Neil Deakin]

Comment on attachment 8854721 [details] [diff] [review]
Focus handler doesn't need to try to move focus if new focused document is being destroyed

Seems ok.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you, Enn.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8854721 [details] [diff] [review]
Focus handler doesn't need to try to move focus if new focused document is being destroyed

I'm not sure who is most familiar with this.

But, mstange is the module owner of view system.
https://wiki.mozilla.org/Modules/Core#View_System

See also comment 16. I'm still not sure why nsViewManager can have outdated nsView pointer as its root view. If you have an idea of that, let me know.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, the patch's comment is same as at testing it on tryserver.

It should be:

> Bug 1342552 Don't try to move focus if the PresShell has gone
> 
> If focus is moving to an element which is in unloading document, should stop moving focus in either nsFocusManager or IMEStateManager because setting focus to such element doesn't make sense.

I'll update it after review before requesting sec-approval.


BTW, due to not sure if the patch gets to fix this bug, I think that it's better to file a normal bug to clean up the focus movement and land the patch normally because somebody could search the security bug from the landed patch but it may not be fixed even with this patch. Then, they might find the STR and some ways to attach with it.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, but even if we'd do so, uplifting this kind of patch is odd for the watchers, sigh...

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Filed bug 1354443 for the life time issue of printing objects.

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8854721 [details] [diff] [review]
Focus handler doesn't need to try to move focus if new focused document is being destroyed

>+  // If new presShell has been destroyed, this should handle as that nobody
>+  // is getting focus.
>+  if (aPresContext &&
>+      (NS_WARN_IF(!aPresContext->GetPresShell()) ||
>+       NS_WARN_IF(aPresContext->GetPresShell()->IsDestroying()))) {
>+    MOZ_LOG(sISMLog, LogLevel::Warning,
>+      ("  OnChangeFocusInternal(), called with destroyed presShell, "
>+       "handling this call as nobody getting focus"));
>+    aPresContext = nullptr;
>+    aContent = nullptr;
>+  }
>+
>   bool focusActuallyChanging =
>     (sContent != aContent || sPresContext != aPresContext ||
>      sActiveTabParent != newTabParent);
> 
>   nsCOMPtr<nsIWidget> oldWidget =
>     sPresContext ? sPresContext->GetRootWidget() : nullptr;
>   if (oldWidget && focusActuallyChanging) {
>     // If we're deactivating, we shouldn't commit composition forcibly because

Aren't we crashing when accessing sPresContext? Not aPresContext? So how would this help?

The patch seems fine as an attempt to fix this crash without knowing the root cause.

Comment 25

[Timothy Nikkel (:tnikkel)]

(In reply to Masayuki Nakano [:masayuki] from comment #23)
> Filed bug 1354443 for the life time issue of printing objects.

Would you mind cc'ing me on that?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Timothy Nikkel (:tnikkel) from comment #25)
> (In reply to Masayuki Nakano [:masayuki] from comment #23)
> > Filed bug 1354443 for the life time issue of printing objects.
> 
> Would you mind cc'ing me on that?

No, I added you.

(In reply to Timothy Nikkel (:tnikkel) from comment #24)
> Comment on attachment 8854721 [details] [diff] [review]
> Focus handler doesn't need to try to move focus if new focused document is
> being destroyed
> 
> >+  // If new presShell has been destroyed, this should handle as that nobody
> >+  // is getting focus.
> >+  if (aPresContext &&
> >+      (NS_WARN_IF(!aPresContext->GetPresShell()) ||
> >+       NS_WARN_IF(aPresContext->GetPresShell()->IsDestroying()))) {
> >+    MOZ_LOG(sISMLog, LogLevel::Warning,
> >+      ("  OnChangeFocusInternal(), called with destroyed presShell, "
> >+       "handling this call as nobody getting focus"));
> >+    aPresContext = nullptr;
> >+    aContent = nullptr;
> >+  }
> >+
> >   bool focusActuallyChanging =
> >     (sContent != aContent || sPresContext != aPresContext ||
> >      sActiveTabParent != newTabParent);
> > 
> >   nsCOMPtr<nsIWidget> oldWidget =
> >     sPresContext ? sPresContext->GetRootWidget() : nullptr;
> >   if (oldWidget && focusActuallyChanging) {
> >     // If we're deactivating, we shouldn't commit composition forcibly because
> 
> Aren't we crashing when accessing sPresContext? Not aPresContext? So how
> would this help?

Ah, good point. It's just my mistake.

> The patch seems fine as an attempt to fix this crash without knowing the
> root cause.

Thanks. Although, we should find the root cause in the other bugs.

Comment 27

[Timothy Nikkel (:tnikkel)]

So were you going to update the patch to remove that bit? Or to check sPresContext instead?

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Timothy Nikkel (:tnikkel) from comment #27)
> So were you going to update the patch to remove that bit? Or to check
> sPresContext instead?

Yeah, I was. But I realized that we can take safer patch here.

If this occurs only with sPresContext->GetRootWidget(), we can cache the widget when it causes sPresContext. I think that such patch could be perfect wallpaper for this crash since GetRootWidget() won't be used with sPresContext.

Comment 29

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8854721 [details] [diff] [review]
Focus handler doesn't need to try to move focus if new focused document is being destroyed

Clearing review in anticipation of new patch.

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: IMEStateManager should cache nsIWidget for sPresContext and use it

Sorry for the delay due to avoiding existing bug of IMEStateManager::UpdateIMEState() callers.

This patch makes IMEStateManager use GetRootWidget() of sPresContext and destroyed PresContext.  So, this must be perfect wallpaper for this crash even though this doesn't fix the actual bug around nsView or printing objects lifetiem management.

This patch looks big, but the changes are simple. Using IMEStateManager::sWidget when sPresContext::GetRootWidget() is necessary.

Other changes are:
* Avoiding the bug of UpdateIMEState() callers. They shouldn't call it before IMEStateManager::OnFocusChange(), but they do it in some cases. I'll file follow up bug for it.
* Using nsCOMPtr<nsIWidget> before calling some methods of nsIWidget.

Comment 31

[Timothy Nikkel (:tnikkel)]

The patch is all in IMEStateManager, which I've never looked at before. Is there a better reviewer?

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I'll ask its review to smaug.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857973 [details] [diff] [review]
IMEStateManager should cache nsIWidget for sPresContext and use it

I'm worried that nothing keeps sWidget alive when it is being used.
Shouldn't we take a strong reference to it before using it?

Ensuring that sWidget never dies at unexpected time is really hard from this patch. And there even are checks that it may die during some calls
+    NotifyIME(REQUEST_TO_COMMIT_COMPOSITION, sWidget);
+    if (NS_WARN_IF(!sWidget)) {

So, looks rather scary.

Strong reference before using sWidget and then perhaps some flag in nsIWidget telling whether it has been destroyed? (nsBaseWidget::Destroy())

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: IMEStateManager should cache nsIWidget for sPresContext and use it

Thanks.

And I changed that IMEStateManager uses nsCOMPtr<nsIWidget> widget(sWidget); even before calling nsIWidget::GetInputContext() because it does not change anything in normal cases.  However, it may call Windows API. Then, other application can do anything if they use API hook. So, it could cause destroying the widget (although, it shouldn't occur in the real world).

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8859542 [details] [diff] [review]
IMEStateManager should cache nsIWidget for sPresContext and use it

>+  // If new aPresShell has been destroyed, this should handle the focus change
>+  // as nobody getting focus.
as nobody is getting focus

>+  // If it's an HTML editor, nsIEditor::PostCreate() may be called before
>+  // a call of IMEStateManager::OnFocusChange().  If it's a plain text editor,
>+  // nsIEditor::SetFlags() may becalled before a call of it too.  In such
Don't quite understand the sentence.
"may be called before" something (I'm not sure what). Clarify this a bit.
>   // This method updates the current IME state.  However, if the enabled state
>   // isn't changed by the new state, this method does nothing.
>   // Note that this method changes the IME state of the active element in the
>   // widget.  So, the caller must have focus.
>   static void UpdateIMEState(const IMEState &aNewIMEState,
>                              nsIContent* aContent,
>-                             nsIEditor* aEditor);
>+                             EditorBase& aEditorBase);
It is unclear to me why & and not *.
But shouldn't matter much if it is guaranteed that aEditorBase stays alive during the method call.
> 
>+  /**
>+   * CanHandleWith() returns false if aPresContext is nullptr or it's destroyed.
>+   */
>+  static bool CanHandleWith(nsPresContext* aPresContext);

ok, so it deal with null.
Then the explicit null check can be removed in places like
>+  if (NS_WARN_IF(!aPresContext) ||
>+      NS_WARN_IF(!CanHandleWith(aPresContext))) {


>+
>+  // sContent and sPresContext are the focused content and presShell. 
Certainly aPresContext isn't a presshell

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #35)
> >+  // If it's an HTML editor, nsIEditor::PostCreate() may be called before
> >+  // a call of IMEStateManager::OnFocusChange().  If it's a plain text editor,
> >+  // nsIEditor::SetFlags() may becalled before a call of it too.  In such
> Don't quite understand the sentence.
> "may be called before" something (I'm not sure what). Clarify this a bit.

Perhaps, this should be clearer:

  // IMEStateManager::UpdateIMEState() should be called after
  // IMEStateManager::OnChangeFocus() is called for setting focus to aContent
  // and aEditorBase.  However, when aEditorBase is an HTMLEditor, this may be
  // called by nsIEditor::PostCreate() before IMEStateManager::OnChangeFocus().
  // Similarly, when aEditorBase is a TextEditor, this may be called by
  // nsIEditor::SetFlags().  In such cases, this method should do nothing
  // because input context should be updated when
  // IMEStateManager::OnChangeFocus() is called later.

> >   // This method updates the current IME state.  However, if the enabled state
> >   // isn't changed by the new state, this method does nothing.
> >   // Note that this method changes the IME state of the active element in the
> >   // widget.  So, the caller must have focus.
> >   static void UpdateIMEState(const IMEState &aNewIMEState,
> >                              nsIContent* aContent,
> >-                             nsIEditor* aEditor);
> >+                             EditorBase& aEditorBase);
> It is unclear to me why & and not *.
> But shouldn't matter much if it is guaranteed that aEditorBase stays alive
> during the method call.

Yes, the callers are methods of nsIEditor. So, their callers should guarantee the editor instance won't be destroyed.

> >+  /**
> >+   * CanHandleWith() returns false if aPresContext is nullptr or it's destroyed.
> >+   */
> >+  static bool CanHandleWith(nsPresContext* aPresContext);
> 
> ok, so it deal with null.
> Then the explicit null check can be removed in places like
> >+  if (NS_WARN_IF(!aPresContext) ||
> >+      NS_WARN_IF(!CanHandleWith(aPresContext))) {

Ah, good point! Thanks!

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: IMEStateManager should cache nsIWidget for sPresContext and use it

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't know because we have no idea to reproduce this crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No. This patch does NOT fix the root problem of the crash (deleted nsView use). This patch completely avoids to use nsView of cached PresContext with caching widget for PresContext directly.

Which older supported branches are affected by this flaw?

Not sure, but not recent regression.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch can be applied to Nightly, Aurora and Beta. I'll attach a patch for ESR52.

How likely is this patch to cause regressions; how much testing does it need?

Not so risky even though this patch looks big. This patch avoids to use PresContext::GetRootWidget() with cached PresContext and makes IMEStateManager caches root widget for focused PresContext. Therefore, this patch modifies a lot of lines to use the cache, but the changes are simple.

Comment 38

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for ESR52

Comment 39

[Al Billings [:abillings - ex-MoCo]]

Sec-approval+ for checkin on *May 3* or later, two weeks into the new cycle. We'll want this back ported so please nominate for all affected branches as well.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

[Tracking Requested - why for this release]:
Same crash reports come from any active branches including ESR45.

I don't think that we should land this patch into ESR45 due to too big and becoming EOL (IIRC). However, it should be decided by security team. If it's necessary for ESR45 too. I'll prepare to write the patch for ESR45 too.

For the other branches, we should take it because this is sec-high.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Al Billings [:abillings] from comment #39)
> Sec-approval+ for checkin on *May 3* or later, two weeks into the new cycle.
> We'll want this back ported so please nominate for all affected branches as
> well.

Oh, I realized that the first week of May is a holiday week of Japan. So, I'd like somebody to land it on May 3.

Comment 42

[Gerry Chang [:gchang]]

Track 54+/55+ as sec-high.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e75e46b355b9dbdfb2d3fea9a4f074dd3143d39

There are no further planned ESR45 releases, so setting that branch to wontfix.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8859910 [details] [diff] [review]
IMEStateManager should cache nsIWidget for sPresContext and use it

Approval Request Comment
[Feature/Bug causing the regression]: unknown, but very old
[User impact if declined]: exploitable crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: per comment 37, it's a big patch but the changes are mechanical and the change made is straightforward
[String changes made/needed]: none

I've confirmed that this grafts cleanly to Beta.

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7e75e46b355b

Comment 46

[Gerry Chang [:gchang]]

Comment on attachment 8859910 [details] [diff] [review]
IMEStateManager should cache nsIWidget for sPresContext and use it

Fix a sec-high. Beta54+ & ESR52+. Should be in 54 beta 6.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3bd4c701447e
https://hg.mozilla.org/releases/mozilla-esr52/rev/ad995e90916b