1252058 - [mac] "New Folder" button in the bookmarks modal dialog hangs Firefox if Sync is enabled.

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[ovidiu boca[:Ovidiu]]

Attachment: bug "new folder".mp4

[Affected versions]:
Firefox Nightly 47.0a1(2016-02-25)

[Affected platforms]:
Mac OS X 10.10

[Steps to reproduce]:

1. Open Nightly with a new profile and sign in with a Firefox account.
2. Right click on a link and choose "Bookmark this link". -> The New bookmark dialog opens.
3. Click on "Show all the bookmarks folders" down arrow.
4. Click on the "New folder" button several times to create new folders.

[Expected result]:

On every click on the "New Folder" button a new folder is created.

[Actual result]:

After 3-4 clicks the browser freezes, to close it you need to force quit.

[Regression range]:
Last good revision: 5b2baa5e9356644a7ed0b73e422eaff62e159ffb (2016-02-24)
First bad revision: c1e0d1890cfee9d86c8d566b0490053f21e0afc6 (2016-02-25)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5b2baa5e9356644a7ed0b73e422eaff62e159ffb&tochange=c1e0d1890cfee9d86c8d566b0490053f21e0afc6

[Additional notes]:
This behavior is reproduce it only on Mac OS, on other operating systems the button "new folder" becomes unavailable. You can see bug 1250445
Please see the attachement.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

The stack looks a little like bug 1004648, and we seem to be spinning (ie, cpu of the parent process is 100%). I'm not sure how to diagnose this further.

The trace:
* thread #1: tid = 0x7a56b, 0x00007fff92390bdc libsystem_pthread.dylib`_pthread_mutex_lock_slow + 241, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
  * frame #0: 0x00007fff92390bdc libsystem_pthread.dylib`_pthread_mutex_lock_slow + 241
    frame #1: 0x00007fff98943d56 CoreFoundation`_CFRunLoopGet2 + 22
    frame #2: 0x00007fff8c66096e Foundation`+[NSRunLoop(NSRunLoop) currentRunLoop] + 30
    frame #3: 0x0000000103b4fb07 XUL`nsAppShell::ProcessNextNativeEvent(this=0x000000010d283ee0, aMayWait=false) + 103 at nsAppShell.mm:542
    frame #4: 0x0000000103af6213 XUL`nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool) [inlined] nsBaseAppShell::DoProcessNextNativeEvent(this=0x000000010d283ee0, mayWait=<unavailable>) + 46 at nsBaseAppShell.cpp:138
    frame #5: 0x0000000103af61e5 XUL`nsBaseAppShell::OnProcessNextEvent(this=0x000000010d283ee0, thr=0x0000000100705050, mayWait=true) + 197 at nsBaseAppShell.cpp:271
    frame #6: 0x0000000103b5006b XUL`nsAppShell::OnProcessNextEvent(this=<unavailable>, aThread=<unavailable>, aMayWait=<unavailable>) + 75 at nsAppShell.mm:746
    frame #7: 0x0000000101cbb3ee XUL`nsThread::ProcessNextEvent(this=0x0000000100705050, aMayWait=true, aResult=0x00007fff5fbf72df) + 798 at nsThread.cpp:987
    frame #8: 0x0000000101ce4033 XUL`NS_ProcessNextEvent(aThread=<unavailable>, aMayWait=true) + 51 at nsThreadUtils.cpp:297
    frame #9: 0x00000001041c8f36 XUL`nsXULWindow::ShowModal(this=0x0000000113ae2020) + 470 at nsXULWindow.cpp:406
    frame #10: 0x000000010418b050 XUL`nsWindowWatcher::OpenWindowInternal(this=<unavailable>, aParent=<unavailable>, aUrl=<unavailable>, aName=<unavailable>, aFeatures=<unavailable>, aCalledFromJS=<unavailable>, aDialog=<unavailable>, aNavigate=<unavailable>, aOpeningTab=<unavailable>, aArgv=<unavailable>, aResult=<unavailable>) + 5472 at nsWindowWatcher.cpp:1058
    frame #11: 0x000000010418bad6 XUL`non-virtual thunk to nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsITabParent*, nsISupports*, mozIDOMWindowProxy**) [inlined] nsWindowWatcher::OpenWindow2(aOpeningTab=<unavailable>, aArguments=<unavailable>, aResult=<unavailable>) + 77 at nsWindowWatcher.cpp:445
    frame #12: 0x000000010418ba89 XUL`non-virtual thunk to nsWindowWatcher::OpenWindow2(this=<unavailable>, aParent=0x0000000112f08420, aUrl=0x000000010fe31248, aName=0x0000000000000000, aFeatures=0x00007fff5fbf79f0, aCalledFromScript=true, aDialog=<unavailable>, aNavigate=<unavailable>, aOpeningTab=<unavailable>, aArguments=<unavailable>, aResult=<unavailable>) + 121 at nsWindowWatcher.cpp:448
    frame #13: 0x0000000102833cef XUL`nsGlobalWindow::OpenInternal(this=0x0000000112f08400, aUrl=0x00007fff5fbf7d08, aName=0x00007fff5fbf7c78, aOptions=0x00007fff5fbf7be8, aDialog=true, aContentModal=<unavailable>, aCalledNoScript=<unavailable>, aDoJSFixups=<unavailable>, aNavigate=<unavailable>, argv=<unavailable>, aExtraArgument=<unavailable>, aJSCallerContext=<unavailable>, aReturn=<unavailable>) + 1471 at nsGlobalWindow.cpp:11492
    frame #14: 0x0000000102834340 XUL`nsGlobalWindow::OpenDialogOuter(this=0x0000000112f08400, aCx=0x0000000100784800, aUrl=0x00007fff5fbf7d08, aName=0x00007fff5fbf7c78, aOptions=0x00007fff5fbf7be8, aExtraArgument=<unavailable>, aError=0x00007fff5fbf7ba8) + 224 at nsGlobalWindow.cpp:7861
    frame #15: 0x0000000102834513 XUL`nsGlobalWindow::OpenDialog(this=<unavailable>, aCx=0x0000000100784800, aUrl=0x00007fff5fbf7d08, aName=<unavailable>, aOptions=<unavailable>, aExtraArgument=<unavailable>, aError=<unavailable>) + 291 at nsGlobalWindow.cpp:7879
    frame #16: 0x000000010303aac8 XUL`mozilla::dom::WindowBinding::openDialog(cx=0x0000000100784800, self=0x0000000113abcc00, args=0x00007fff5fbf7eb0, obj=<unavailable>) + 600 at WindowBinding.cpp:5761
    frame #17: 0x000000010302fc28 XUL`mozilla::dom::WindowBinding::genericMethod(cx=<unavailable>, argc=<unavailable>, vp=<unavailable>) + 392 at WindowBinding.cpp:13709
    frame #18: 0x000000010504df76 XUL`js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [inlined] js::CallJSNative(cx=0x0000000100784800, native=<unavailable>)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 341 at jscntxtinlines.h:235
    frame #19: 0x000000010504de21 XUL`js::Invoke(cx=0x0000000100784800, args=0x00007fff5fbf7fd0, construct=<unavailable>) + 289 at Interpreter.cpp:478
    frame #20: 0x000000010504e60e XUL`js::Invoke(cx=0x0000000100784800, thisv=<unavailable>, fval=<unavailable>, argc=<unavailable>, argv=<unavailable>, rval=<unavailable>) + 430 at Interpreter.cpp:530
    frame #21: 0x0000000104f7693f XUL`js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const [inlined] js::DirectProxyHandler::call(args=<unavailable>) const + 67 at DirectProxyHandler.cpp:77
    frame #22: 0x0000000104f768fc XUL`js::CrossCompartmentWrapper::call(this=<unavailable>, cx=0x0000000100784800, args=0x00007fff5fbf8168, wrapper=<unavailable>) const + 252 at CrossCompartmentWrapper.cpp:289
    frame #23: 0x0000000104f7ab86 XUL`js::Proxy::call(cx=<unavailable>, args=<unavailable>, proxy=<unavailable>) + 214 at Proxy.cpp:391
    frame #24: 0x0000000104f7b5aa XUL`js::proxy_Call(cx=<unavailable>, argc=<unavailable>, vp=<unavailable>) + 106 at Proxy.cpp:683
    frame #25: 0x000000010504e2e3 XUL`js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [inlined] js::CallJSNative(cx=0x0000000100784800, native=0x0000000104f7b540)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 240 at jscntxtinlines.h:235
    frame #26: 0x000000010504e1f3 XUL`js::Invoke(cx=0x0000000100784800, args=0x00007fff5fbf83f8, construct=<unavailable>) + 1267 at Interpreter.cpp:466
    frame #27: 0x0000000105045f8a XUL`Interpret(cx=0x0000000100784800, state=0x00007fff5fbf8bd0) + 62570 at Interpreter.cpp:2802
    frame #28: 0x0000000105036aed XUL`js::RunScript(cx=0x0000000100784800, state=0x00007fff5fbf8bd0) + 413 at Interpreter.cpp:428
    frame #29: 0x000000010504e422 XUL`js::Invoke(cx=0x0000000100784800, args=<unavailable>, construct=<unavailable>) + 1826 at Interpreter.cpp:496
    frame #30: 0x000000010504e60e XUL`js::Invoke(cx=0x0000000100784800, thisv=<unavailable>, fval=<unavailable>, argc=<unavailable>, argv=<unavailable>, rval=<unavailable>) + 430 at Interpreter.cpp:530
    frame #31: 0x0000000104f7693f XUL`js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const [inlined] js::DirectProxyHandler::call(args=<unavailable>) const + 67 at DirectProxyHandler.cpp:77
    frame #32: 0x0000000104f768fc XUL`js::CrossCompartmentWrapper::call(this=<unavailable>, cx=0x0000000100784800, args=0x00007fff5fbf8df8, wrapper=<unavailable>) const + 252 at CrossCompartmentWrapper.cpp:289
    frame #33: 0x0000000104f7ab86 XUL`js::Proxy::call(cx=<unavailable>, args=<unavailable>, proxy=<unavailable>) + 214 at Proxy.cpp:391
    frame #34: 0x0000000104f7b5aa XUL`js::proxy_Call(cx=<unavailable>, argc=<unavailable>, vp=<unavailable>) + 106 at Proxy.cpp:683
    frame #35: 0x000000010504e2e3 XUL`js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [inlined] js::CallJSNative(cx=0x0000000100784800, native=0x0000000104f7b540)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 240 at jscntxtinlines.h:235
    frame #36: 0x000000010504e1f3 XUL`js::Invoke(cx=0x0000000100784800, args=0x00007fff5fbf9088, construct=<unavailable>) + 1267 at Interpreter.cpp:466
    frame #37: 0x0000000105045f8a XUL`Interpret(cx=0x0000000100784800, state=0x00007fff5fbf9860) + 62570 at Interpreter.cpp:2802
    frame #38: 0x0000000105036aed XUL`js::RunScript(cx=0x0000000100784800, state=0x00007fff5fbf9860) + 413 at Interpreter.cpp:428
    frame #39: 0x000000010504e422 XUL`js::Invoke(cx=0x0000000100784800, args=<unavailable>, construct=<unavailable>) + 1826 at Interpreter.cpp:496
    frame #40: 0x000000010504e60e XUL`js::Invoke(cx=0x0000000100784800, thisv=<unavailable>, fval=<unavailable>, argc=<unavailable>, argv=<unavailable>, rval=<unavailable>) + 430 at Interpreter.cpp:530
    frame #41: 0x0000000104f7693f XUL`js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const [inlined] js::DirectProxyHandler::call(args=<unavailable>) const + 67 at DirectProxyHandler.cpp:77
    frame #42: 0x0000000104f768fc XUL`js::CrossCompartmentWrapper::call(this=<unavailable>, cx=0x0000000100784800, args=0x00007fff5fbf9a88, wrapper=<unavailable>) const + 252 at CrossCompartmentWrapper.cpp:289
    frame #43: 0x0000000104f7ab86 XUL`js::Proxy::call(cx=<unavailable>, args=<unavailable>, proxy=<unavailable>) + 214 at Proxy.cpp:391
    frame #44: 0x0000000104f7b5aa XUL`js::proxy_Call(cx=<unavailable>, argc=<unavailable>, vp=<unavailable>) + 106 at Proxy.cpp:683
    frame #45: 0x000000010504e2e3 XUL`js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [inlined] js::CallJSNative(cx=0x0000000100784800, native=0x0000000104f7b540)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 240 at jscntxtinlines.h:235
    frame #46: 0x000000010504e1f3 XUL`js::Invoke(cx=0x0000000100784800, args=0x00007fff5fbf9b60, construct=<unavailable>) + 1267 at Interpreter.cpp:466
    frame #47: 0x000000010504e60e XUL`js::Invoke(cx=0x0000000100784800, thisv=<unavailable>, fval=<unavailable>, argc=<unavailable>, argv=<unavailable>, rval=<unavailable>) + 430 at Interpreter.cpp:530
    frame #48: 0x0000000104ca35fc XUL`js::jit::DoCallFallback(cx=0x0000000100784800, frame=0x00007fff5fbf9fd8, stub_=0x0000000114e3d2a8, argc=1, vp=0x00007fff5fbf9f78, res=<unavailable>) + 1708 at BaselineIC.cpp:6136
    frame #49: 0x000000010a440700
(lldb)

Comment 2

[Mark Hammond [:markh] [:mhammond]]

This is going to be something to do with Sync's nested event loops. I think bug 1210296 would also fix it, but it would be a shame if it has to wait for that...

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Oops - I meant the meta bug 1007448

Comment 4

[Mark Hammond [:markh] [:mhammond]]

While Sync is bad for spinning nested event loops, I think this bug belongs in cocoa (and hopefully this change will get someone's attention :)

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

If we want to enable this feature for 47, SV team would like to see this fixed. Tracked as blocking for Fx47.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Did some digging into this. You don't need to add more than one new folder to cause the bug, just add one, and then wait with the focus over the "New Folder" text (e.g. editing the title of the folder). I think this means the bug is pretty likely to get hit.

It's basically stuck spinning in the modal dialog's event loop, and most meaningful work isn't getting done, and it's not entirely obvious why. It still seems to be dispatching timer events, so ironically, Sync is able to work fine, despite the fact that the entire browser is hung.

Anyway, it looks like this was introduced by the fix for bug 1218032. I honestly don't have a ton more to go on than this. Doing a revert of that bug's changeset does fix the problem, but obviously is bad for other reasons (although I can attach a patch for this if necessary). I suspect it might be due to something that the IMEContentObserver is doing, since when we're hung we go through that code quite frequently, but I'm not exactly sure what the problem is. Any input would be appreciated.

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

[Tracking Requested - why for this release]: I'm confident this was caused by the bug listed above, which would mean it's present in release 46.

Comment 8

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

Oh, this is bad. 
But I'm lost now. The initial comment has regression range which doesn't contain bug 1218032.
Which one is wrong: the regression range or the bug number?

Comment 9

[Thom Chiovoloni [:tcsc] (ex-moco)]

The regression range is wrong.

Comment 10

[Thom Chiovoloni [:tcsc] (ex-moco)]

I tracked it down with mozregression myself, and came out with a different range, which is how I identified bug 1218032.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Brian, Masayuki, could you please address the concern in comment 6? QA deems the hang (this bug) to be a blocking issue for Sync'd tab feature which is planned for Fx47. The investigation reveals that this may be due to the fix from bug 1218032. Could you please help root cause and possibly provide a fix? 47 enters Beta cycle in ~10 days so a prompt follow up is appreciated. Many thanks!

Comment 12

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

Oh, sorry for forgetting to reply the NI. I'm not familiar with modal dialog event loop, so, I put off the answer until checking the modal dialog related code in PresShell.

Comment 13

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

I tested this bug on Windows 10 with non-fresh profile. Then, I cannot reproduce the hang but I cannot edit the folder normally since the folder name is automatically committed in a couple of sec. It seems that what happens depends on native IME handler what does.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #13)
> I tested this bug on Windows 10 with non-fresh profile.

Yeah, this can only be reproduced on Mac.

> Then, I cannot
> reproduce the hang but I cannot edit the folder normally since the folder
> name is automatically committed in a couple of sec. It seems that what
> happens depends on native IME handler what does.

I believe that problem is (also?) due to Sync spinning a nested event loop as soon as the new folder is created - nested event loops have a number of similar issues (eg, open popups may be rolled up in this case too). That specific case is in bug 697649 (but it's not a huge priority at the moment, as the impact is relatively low and we'd rather just get rid of Sync using nested event loops completely)

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like a regression since 44. Only reproducible on Macs, from comment 0. 
Does this affect 46 or is this only dealing with synced tabs?  

Andrei can your team have a look to check on 46 beta 10? Thanks.

Comment 16

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting ni? for Andrei so he sees the above request from Liz.

Comment 17

[Camelia Badau [:cbadau], Desktop Test Engineering]

I've tested on Mac OS X 10.9.5 using Firefox 46 Beta 10 and the issue is reproducible here, with the steps from Description and also with the steps from comment 6: "You don't need to add more than one new folder to cause the bug, just add one, and then wait with the focus over the "New Folder" text (e.g. editing the title of the folder)". It seems that it affects 46 and it's not related to synced tabs. 

I've also tested on Nightly 45.0a1, 09-11-2016 (buildID: 20151109030417) - this is the build that doesn't contain the fix for bug 1218032 - and here the issue isn't reproducible. Like mentioned in comment 6, this bug seems to have been introduced by the fix of bug 1218032.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

I don't think bug 1218032 landed on 46. Jim, do you think this has anything to do with bug 1218032? 
This also doesn't seem limited to synced tabs.   

Mark and Masayuki can you come up with a workaround for 46 so that we don't ship 46 with this regression?  Or can you suggest someone else who may be able to help here?

Comment 19

[Karl Thiessen [:kthiessen, he/him]]

I would suggest rnewman as someone who might be able to help in the absence of markh (due to it already being the weekend in Australia.)

Comment 20

[Mark Hammond [:markh] [:mhammond]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> Mark and Masayuki can you come up with a workaround for 46 so that we don't
> ship 46 with this regression?  Or can you suggest someone else who may be
> able to help here?

Sorry, I can't think of a workaround and can't think of anyone else beyond those already on this bug and bug 1218032.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

rnewman, might you have time to look at this?

Comment 22

[Jim Mathies [:jimm]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> I don't think bug 1218032 landed on 46. Jim, do you think this has anything
> to do with bug 1218032? 
> This also doesn't seem limited to synced tabs.   
> 
> Mark and Masayuki can you come up with a workaround for 46 so that we don't
> ship 46 with this regression?  Or can you suggest someone else who may be
> able to help here?

Those patches originally landed in 44 right before a merge, then were backed out of 44 on aurora. They rolled out in 45, so 46 has them.

Comment 23

[Jim Mathies [:jimm]]

Brian, this is a serious regression in 45 that causes a hung browser. We either need to backout the offending work or get a fix in the works. The preference would be to keep the crash fix.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

We may want to relnote this for 46 as a known issue for mac users.

Comment 25

[Brian Birtles (:birtles)]

Thanks for the summary Jim. Masayuki and I are looking into it now.

Comment 26

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

According to the log of IMEContentObserver, it tries to post selection change notifications infinitely. Although, I'm still not sure why this occurs.

Comment 27

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff3cc78b3e21

Comment 28

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

I can often reproduce this bug. So, I'd like somebody to test the patched build (see comment 27) to confirm my patch fixes this bug.

Comment 29

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #28)
> I can often reproduce this bug. So, I'd like somebody to test the patched
> build (see comment 27) to confirm my patch fixes this bug.

Ovidiu do you have the bandwidth to verify this? Ping me if you need any help with the builds.

Comment 30

[ovidiu boca[:Ovidiu]]

Hi,

I tested the patched build from comment 27, I can still reproduce the issue but the AR are different: after 3-4 clicks the "New Folder" button loses its focus (becomes gray out) and only clicking again the "New Folder" the focus is regained and you can add new folders. 


Florin thanks for help.

Comment 31

[Mark Hammond [:markh] [:mhammond]]

(In reply to ovidiu boca[:Ovidiu] from comment #30)
> I tested the patched build from comment 27, I can still reproduce the issue
> but the AR are different: after 3-4 clicks the "New Folder" button loses its
> focus (becomes gray out) and only clicking again the "New Folder" the focus
> is regained and you can add new folders. 

That's the long-standing bug 697649, apparently caused by Sync's nested event-loop spinning. Given this bug is about Firefox hanging, I think we can treat this as "fixed by that patch".

Comment 32

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b85c77e3575d

Comment 33

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

Attachment: MozReview Request: Bug 1252058 IMEContentObserver::IMENotificationSender shouldn't post notifications when IMEContentObserver which is the owner of it stopped observing contents r?smaug

When IMEContentObserver stopped observing contents, posting pending notifications to current thread may cause infinite loop because it's impossible to send notifications to widget until the IMEContentObserver is reinitialized.

When IMEContentObserver is reinitialized, pending notifications are automatically flushed.  So, in such case, IMEContentObserver::IMEnotificationSender shouldn't clear the pending notifications but don't post the notifications to current thread immediately.

Review commit: https://reviewboard.mozilla.org/r/47649/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47649/

Comment 34

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

Comment on attachment 8743213 [details]
MozReview Request: Bug 1252058 IMEContentObserver::IMENotificationSender shouldn't post notifications when IMEContentObserver which is the owner of it stopped observing contents r?smaug

https://reviewboard.mozilla.org/r/47649/#review45289

Sorry about review delay

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a13c98a57925

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a13c98a57925

Comment 37

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c97bcd532ad

Comment 38

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

Comment on attachment 8743213 [details]
MozReview Request: Bug 1252058 IMEContentObserver::IMENotificationSender shouldn't post notifications when IMEContentObserver which is the owner of it stopped observing contents r?smaug

Approval Request Comment
[Feature/regressing bug #]: Bug 1218032
[User impact if declined]: May meet the hang up without this patch.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Low. This makes IMENotificationSender post notifications to current thread if its parent IMEContentObserver stopped working. Even if there are stopped notifications after IMEContentObserver restarts to observe editor contents, this patch makes IMEContentObserver tries to notify IME of latest content information at that time.
[String/UUID change made/needed]: Nothing.

Comment 39

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Ovidiu, could you please verify this hang is gone as expected on a latest Nightly build? That would help me immensely when deciding to uplift this to Beta47? Thanks!

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Florin, this is a blocking bug for Fx47 so the sooner we can verify the fix the better for 47 release health. Just fyi.

Comment 41

[ovidiu boca[:Ovidiu]]

Hi Ritu, 

I have tested this issue on Mac OS X 10.10 using FF Nightly 49.0a1(2016-04-27) and I couldn't reproduce the browser freezing. Just FYI the other issue from bug 697649 is still there. It will be ok if we deal separately this 2 issues?

Comment 42

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to ovidiu boca[:Ovidiu] from comment #41)
> Hi Ritu, 
> 
> I have tested this issue on Mac OS X 10.10 using FF Nightly
> 49.0a1(2016-04-27) and I couldn't reproduce the browser freezing. Just FYI
> the other issue from bug 697649 is still there. It will be ok if we deal
> separately this 2 issues?

Thanks for the verification. Bug 697649 is an old bug. Yes we should treat them as separate issues. I will consider this one as verified.

Comment 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8743213 [details]
MozReview Request: Bug 1252058 IMEContentObserver::IMENotificationSender shouldn't post notifications when IMEContentObserver which is the owner of it stopped observing contents r?smaug

Fix was verified on Nightly and this was deemed blocking for Fx47, Beta47+

Comment 44

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

https://hg.mozilla.org/releases/mozilla-beta/rev/9fad204a1b6e

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I don't think we ever added this to 46 relnotes as a known issue. This is fixed in F47+. Clearing the relnote flag.