Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Description • 12 years ago

While trying to edit some text inside a <input type="text">, chances are we can get this error

  [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIEditor.removeEditorObserver]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://browser/content/forms.js :: fa_setFocusedElement :: line 238" data: no]

It seems we are trying to remove a observer that was never added.

One obvious error is that at https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/forms.js#263 must start with lower case, but there are other unknown problems.

/me trying to make a minimum test page

	Jason Smith [:jsmith]
	Comment 1 • 12 years ago

Actually before I nom this, we need to understand the user impact.

What's the user impact of this?

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 2 • 12 years ago

(In reply to Kan-Ru Chen [:kanru] from comment #0)
> One obvious error is that at
> https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/forms.
> js#263 must start with lower case, but there are other unknown problems.

Actually I'm wrong about this one. The interface https://mxr.mozilla.org/mozilla-central/source/editor/idl/nsIEditorObserver.idl actually uses CamelCase so in the js land we have to use it too.

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 3 • 12 years ago

(In reply to Jason Smith [:jsmith] from comment #1)
> Actually before I nom this, we need to understand the user impact.
> 
> What's the user impact of this?

Unable to open the keyboard. I need to find a way to produce though.

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 4 • 12 years ago

BTW, this is on m-c only.

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 5 • 11 years ago

(In reply to Kan-Ru Chen [:kanru] from comment #0)
> While trying to edit some text inside a <input type="text">, chances are we
> can get this error
> 
>   [Exception... "Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsIEditor.removeEditorObserver]" nsresult: "0x80004005
> (NS_ERROR_FAILURE)" location: "JS frame :: chrome://browser/content/forms.js
> :: fa_setFocusedElement :: line 238" data: no]
> 
> It seems we are trying to remove a observer that was never added.
> 
There is a bug with `nsIEditor.removeEditorObserver`. Sometimes it fails to remove the observer after a successful `nsIEditor.addEditorObserver`, which can be produced by the test case of Bug 889138.

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 6 • 11 years ago

The patch catches the exception of `nsIEditor.removeEditorObserver exception` to avoid breaking the form.js.

	Jason Smith [:jsmith]
	Comment 7 • 11 years ago

Point of clarification - so this fix is only for m-c only, even though one of the blocking bugs is leo+? Trying to figure out if we need a blocking flag set here.

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 8 • 11 years ago

We need set leo+ flag for this fix and make sure we will fix b2g18 as well.

	bhavana bajaj [:bajaj]
	Comment 9 • 11 years ago

(In reply to Yuan Xulei [:yxl] from comment #8)
> We need set leo+ flag for this fix and make sure we will fix b2g18 as well.

Bug 860546 is already resolved, which bug is this one blocking ?

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 10 • 11 years ago

(In reply to Yuan Xulei [:yxl] from comment #5)
> (In reply to Kan-Ru Chen [:kanru] from comment #0)
> > While trying to edit some text inside a <input type="text">, chances are we
> > can get this error
> > 
> >   [Exception... "Component returned failure code: 0x80004005
> > (NS_ERROR_FAILURE) [nsIEditor.removeEditorObserver]" nsresult: "0x80004005
> > (NS_ERROR_FAILURE)" location: "JS frame :: chrome://browser/content/forms.js
> > :: fa_setFocusedElement :: line 238" data: no]
> > 
> > It seems we are trying to remove a observer that was never added.
> > 
> There is a bug with `nsIEditor.removeEditorObserver`. Sometimes it fails to
> remove the observer after a successful `nsIEditor.addEditorObserver`, which
> can be produced by the test case of Bug 889138.

How? Are you sure the observer was added before we call removeEditorObserver? The implementation of removeEditorObserver calls nsCOMArray::RemoveObject directly. If that could go wrong then we have a bigger problem here.

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 11 • 11 years ago

(In reply to Kan-Ru Chen [:kanru] from comment #10) 
> How? Are you sure the observer was added before we call
> removeEditorObserver? The implementation of removeEditorObserver calls
> nsCOMArray::RemoveObject directly. If that could go wrong then we have a
> bigger problem here.

Yes, I'm sure of it.

I tested with this patch. 

In the forms.js, there only exists one line of code to add observer and one line to remove observer. The patch prints logs on these lines.

When `_editor` is created and assigned,  `addEditorObserver` will be called to add `FormAssistant` as
observer and output the log:
  FormAssistant_ID === add

When `_editor` is released, `removeEditorObserver` will be called and output the log:
  FormAssistant_ID === removed

Open the music app, click the search field to launch the keyboard and then cancel it. I got the following logs:

0.4138290514395244 === add 
0.31306894307620836 === add 
0.4138290514395244 == remove 
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIEditor.removeEditorObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/forms.js :: fa_setFocusedElement :: line 243"  data: no]
0.31306894307620836 == remove 
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIEditor.removeEditorObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/forms.js :: fa_setFocusedElement :: line 243"  data: no]

Form the log I can see that the FormAssistant of ID 0.4138290514395244 was added, but failed to remove.

	Alex Keybl [:akeybl]
	Comment 12 • 11 years ago

Please renominate if you are able to reliably reproduce on 1.1, which it isn't clear from this bug.

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 13 • 11 years ago

I suspect it's an issue of object wrapping. I'm trying to add logs to nsIEditor and see what I could find.

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 14 • 11 years ago

3153:I/Gecko   ( 6081): LOG trying to add editor observer 0x44bea828
3154:I/Gecko   ( 6081): LOG editor observer added
3155:I/Gecko   ( 6081): LOG find it at: 0
3510:I/Gecko   ( 6081): LOG trying to add editor observer 0x449a9990
3511:I/Gecko   ( 6081): LOG editor observer added
3512:I/Gecko   ( 6081): LOG find it at: 1
3513:I/Gecko   ( 6081): LOG trying to remove editor observer 0x44bea828
3514:I/Gecko   ( 6081): LOG try find it at: 0
3515:I/Gecko   ( 6081): LOG trying to add editor observer 0x43f8f708
3516:I/Gecko   ( 6081): LOG editor observer added
3517:I/Gecko   ( 6081): LOG find it at: 0
3569:I/Gecko   ( 6081): LOG trying to remove editor observer 0x44925950
3570:I/Gecko   ( 6081): LOG try find it at: -1
3574:I/Gecko   ( 6081): LOG trying to remove editor observer 0x43f8f708
3575:I/Gecko   ( 6081): LOG try find it at: 0
3576:I/Gecko   ( 6081): LOG trying to add editor observer 0x43f8fca8
3577:I/Gecko   ( 6081): LOG editor observer added
3578:I/Gecko   ( 6081): LOG find it at: 0
3585:I/Gecko   ( 6081): LOG trying to remove editor observer 0x44b482f0
3586:I/Gecko   ( 6081): LOG try find it at: -1

	Jason Smith [:jsmith]
	Comment 16 • 11 years ago

Now I've got clarification in the other bug on why this bug is needed. See https://bugzilla.mozilla.org/show_bug.cgi?id=889138#c21.

This bug is needed to be fixed in order to fix the bug seen in bug 889138. bug 889138 is already a leo+ blocker and a youtube certification blocker, so this should be a blocker as well.

	Donovan Preston [:fzzzy]
	Comment 17 • 11 years ago

I have confirmed that the attached patch does fix the issue which is blocking YouTube from performing certification. (repo steps in bug 893000) Can we get this landed asap?

	Alex Keybl [:akeybl]
	Comment 18 • 11 years ago

Comment on attachment 773896 [details] [diff] [review]
Suppress Supress nsIEditor.removeEditorObserver exception

Donovan, just needs review. Can you help with that? It's a simple try/catch.

	Donovan Preston [:fzzzy]
	Comment 19 • 11 years ago

Comment on attachment 773896 [details] [diff] [review]
Suppress Supress nsIEditor.removeEditorObserver exception

Review of attachment 773896 [details] [diff] [review]:
-----------------------------------------------------------------

Simple try/catch to squelch the exception. I tested the patch, it fixes YouTube's issue.

	Chris Kreinbring [:CKreinbring]
	Comment 20 • 11 years ago

Still repros in Leo 1.1 commercial RIL.

Build ID: 20130715070218
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/6062fdf2deb8
Gaia: 55ed5e08a2250ea2d3571fff860c39e66fabed14
Platform Version: 18.1
RIL Version: 01.01.00.019.158

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 21 • 11 years ago

Kan-Ru, could you check in the patch for temporary workaround for the youtube problem?
We can continue to investigate the issue of nsIEditor.

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 22 • 11 years ago

Comment on attachment 773896 [details] [diff] [review]
Suppress Supress nsIEditor.removeEditorObserver exception

Review of attachment 773896 [details] [diff] [review]:
-----------------------------------------------------------------

I believe the workaround is incorrect and could potentially leak memory. If the observer could disappear unnoticed then the entire logic surrounding it might also not work as expected. I suggest you to backout that part instead.

	Jason Smith [:jsmith]
	Comment 23 • 11 years ago

removing checkin-needed per comment 22

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 24 • 11 years ago

Blake, do you know why we could get different pointer addresses in the nsEditor::RemoveEditorObserver method?

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 25 • 11 years ago

(In reply to Kan-Ru Chen [:kanru] from comment #22)
> Comment on attachment 773896 [details] [diff] [review]
> Suppress Supress nsIEditor.removeEditorObserver exception
> 
> Review of attachment 773896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I believe the workaround is incorrect and could potentially leak memory. If
> the observer could disappear unnoticed then the entire logic surrounding it
> might also not work as expected. I suggest you to backout that part instead.

Though the observer can not be removed sometimes, the editor holding the observers will be freed. So the chance of leaking memory is little. I reluctant to agree with the idea of backouting the editor observer part, which would break another leo+ blocker Bug 860546.

	Yuan Xulei [:yxl] (Leave from May 18)
	Comment 26 • 11 years ago

Kan-Ru, can you help to take the bug as I'm on travelling and won't have sufficient to work on it until I'm back to Bejing on Wednesday night?

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 27 • 11 years ago

Yeah, I will try to resolve this bug as soon as possible. Or deploy your workaronud if I can't make it.

	Dylan Oliver [:doliver]
	Comment 28 • 11 years ago

Thanks Kan-Ru -- this one is high priority to unblock YouTube and we are hoping to land a fix in the in next day or two if possible.

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 29 • 11 years ago

So the root cause is that when the nsIFrame of the input element is reconstructed by CSS styling, the editor observers are removed. Need to find out if we have to reattach the observers in this case.

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 30 • 11 years ago

#0  ~nsXPCWrappedJS (this=0x43fadb40, __in_chrg=<value optimized out>) at /home/kanru/zone2/mozilla/central/js/xpconnect/src/XPCWrappedJS.cpp:453
#1  0x41257110 in nsXPCWrappedJS::Release (this=0x43fadb40) at /home/kanru/zone2/mozilla/central/js/xpconnect/src/XPCWrappedJS.cpp:204
#2  0x408a54ea in nsXPTCStubBase::Release (this=<value optimized out>) at /home/kanru/zone2/mozilla/central/xpcom/reflect/xptcall/src/xptcall.cpp:36
#3  0x4085088e in ReleaseObjects (aArray=...) at /home/kanru/zone2/mozilla/B2G/objdir-gecko-central-debug/xpcom/build/nsCOMArray.cpp:243
#4  0x408512ae in nsCOMArray_base::Clear (this=<value optimized out>) at /home/kanru/zone2/mozilla/B2G/objdir-gecko-central-debug/xpcom/build/nsCOMArray.cpp:251
#5  0x4104be30 in nsEditor::PreDestroy (this=0x44c8c2c0, aDestroyingFrames=true) at /home/kanru/zone2/mozilla/central/editor/libeditor/base/nsEditor.cpp:455
#6  0x40e007d8 in nsTextEditorState::DestroyEditor (this=0x447e87e0) at /home/kanru/zone2/mozilla/central/content/html/content/src/nsTextEditorState.cpp:1419
#7  0x40e046c2 in nsTextEditorState::UnbindFromFrame (this=0x447e87e0, aFrame=<value optimized out>) at /home/kanru/zone2/mozilla/central/content/html/content/src/nsTextEditorState.cpp:1457
#8  0x40d9c148 in mozilla::dom::HTMLInputElement::UnbindFromFrame (this=<value optimized out>, aFrame=0x444c2e78) at /home/kanru/zone2/mozilla/central/content/html/content/src/HTMLInputElement.cpp:2009
#9  0x40afc3a0 in nsTextControlFrame::DestroyFrom (this=0x444c2e78, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/forms/nsTextControlFrame.cpp:159
#10 0x40b472d2 in nsLineBox::DeleteLineList (aPresContext=<value optimized out>, aLines=..., aDestructRoot=0x444c1010, aFrames=0x44c7228c) at /home/kanru/zone2/mozilla/central/layout/generic/nsLineBox.cpp:375
#11 0x40b06984 in nsBlockFrame::DestroyFrom (this=0x44c72250, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsBlockFrame.cpp:283
#12 0x40b2d782 in nsFrameList::DestroyFramesFrom (this=0x444c25dc, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsFrameList.cpp:57
#13 0x40b1603a in nsContainerFrame::DestroyFrom (this=0x444c25a0, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsContainerFrame.cpp:252
#14 0x40b3519a in nsHTMLScrollFrame::DestroyFrom (this=0x444c25a0, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsGfxScrollFrame.cpp:102
#15 0x40b472d2 in nsLineBox::DeleteLineList (aPresContext=<value optimized out>, aLines=..., aDestructRoot=0x444c1010, aFrames=0x44caab3c) at /home/kanru/zone2/mozilla/central/layout/generic/nsLineBox.cpp:375
#16 0x40b06984 in nsBlockFrame::DestroyFrom (this=0x44caab00, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsBlockFrame.cpp:283
#17 0x40b472d2 in nsLineBox::DeleteLineList (aPresContext=<value optimized out>, aLines=..., aDestructRoot=0x444c1010, aFrames=0x44caa39c) at /home/kanru/zone2/mozilla/central/layout/generic/nsLineBox.cpp:375
#18 0x40b06984 in nsBlockFrame::DestroyFrom (this=0x44caa360, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsBlockFrame.cpp:283
#19 0x40b2d782 in nsFrameList::DestroyFramesFrom (this=0x444c104c, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsFrameList.cpp:57
#20 0x40b1603a in nsContainerFrame::DestroyFrom (this=0x444c1010, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsContainerFrame.cpp:252
#21 0x40b3519a in nsHTMLScrollFrame::DestroyFrom (this=0x444c1010, aDestructRoot=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsGfxScrollFrame.cpp:102
#22 0x40b2d744 in nsIFrame::Destroy (this=0x44c2cec0, aFrame=<value optimized out>) at /home/kanru/zone2/mozilla/central/layout/generic/nsIFrame.h:598
#23 nsFrameList::DestroyFrame (this=0x44c2cec0, aFrame=<value optimized out>) at /home/kanru/zone2/mozilla/central/layout/generic/nsFrameList.cpp:138
#24 0x40b0125a in nsAbsoluteContainingBlock::RemoveFrame (this=0x44c2cec0, aDelegatingFrame=<value optimized out>, aListID=<value optimized out>, aOldFrame=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/generic/nsAbsoluteContainingBlock.cpp:111
#25 0x40abbd02 in nsFrameManager::RemoveFrame (this=0x44739c00, aListID=mozilla::layout::kAbsoluteList, aOldFrame=0x444c1010) at /home/kanru/zone2/mozilla/central/layout/base/nsFrameManager.cpp:479
#26 0x40b4ee9a in nsPlaceholderFrame::DestroyFrom (this=0x44caa3c0, aDestructRoot=0x44caa3c0) at /home/kanru/zone2/mozilla/central/layout/generic/nsPlaceholderFrame.cpp:157
#27 0x40b08a0e in nsIFrame::Destroy (this=0x44c8f260, aDeletedFrame=0x44caa3c0, aFlags=2) at /home/kanru/zone2/mozilla/central/layout/generic/nsIFrame.h:598
#28 nsBlockFrame::DoRemoveFrame (this=0x44c8f260, aDeletedFrame=0x44caa3c0, aFlags=2) at /home/kanru/zone2/mozilla/central/layout/generic/nsBlockFrame.cpp:5454
#29 0x40b08cde in nsBlockFrame::RemoveFrame (this=0x44c8f260, aListID=<value optimized out>, aOldFrame=0x44caa3c0) at /home/kanru/zone2/mozilla/central/layout/generic/nsBlockFrame.cpp:5012
#30 0x40abbd1a in nsFrameManager::RemoveFrame (this=0x44739c00, aListID=mozilla::layout::kPrincipalList, aOldFrame=0x44caa3c0) at /home/kanru/zone2/mozilla/central/layout/base/nsFrameManager.cpp:481
#31 0x40a9111c in nsCSSFrameConstructor::ContentRemoved (this=0x44739c00, aContainer=0x447e83d0, aChild=0x447e8470, aOldNextSibling=0x447e8a60, aFlags=nsCSSFrameConstructor::REMOVE_FOR_RECONSTRUCTION, aDidReconstruct=0xbeff9a2f) at /home/kanru/zone2/mozilla/central/layout/base/nsCSSFrameConstructor.cpp:7540
#32 0x40a8ff18 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x44739c00, aContent=0x447e8470, aAsyncInsert=false) at /home/kanru/zone2/mozilla/central/layout/base/nsCSSFrameConstructor.cpp:9342
#33 0x40a9231a in nsCSSFrameConstructor::ProcessRestyledFrames (this=0x44739c00, aChangeList=<value optimized out>) at /home/kanru/zone2/mozilla/central/layout/base/nsCSSFrameConstructor.cpp:8192
#34 0x40a92e68 in nsCSSFrameConstructor::RestyleElement (this=0x44739c00, aElement=<value optimized out>, aPrimaryFrame=0x44c6f4a0, aMinHint=0, aRestyleTracker=..., aRestyleDescendants=true) at /home/kanru/zone2/mozilla/central/layout/base/nsCSSFrameConstructor.cpp:8342

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 31 • 11 years ago

Comment on attachment 773896 [details] [diff] [review]
Suppress Supress nsIEditor.removeEditorObserver exception

As said in comment 29 the observer is removed when the frame is reconstructed. The workaround looks safe now. We still have to check if this is correct for bug 860546.

	[:fabrice] Fabrice Desré
	Comment 32 • 11 years ago

Comment on attachment 773896 [details] [diff] [review]
Suppress Supress nsIEditor.removeEditorObserver exception

Review of attachment 773896 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment fixed.

::: b2g/chrome/content/forms.js
@@ +235,5 @@
>  
>      this._documentEncoder = null;
>      if (this._editor) {
> +      // Catch [nsIEditor.removeEditorObserver] failure exception to work
> +      // around Bug 874754.

Update this comment to remove the bug number and explain why this fails sometimes.

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 33 • 11 years ago

https://hg.mozilla.org/projects/birch/rev/e0e452cfde99

	Kan-Ru Chen [:kanru] (UTC+9) Assignee
	Comment 34 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a49ef62295c5

	Donovan Preston [:fzzzy]
	Comment 35 • 11 years ago

Unfortunately this didn't make it into the nightly build, but I did verify that the tip of the b2g18 branch does fix YouTube's issue.

Thanks!!

	Jason Smith [:jsmith]
	Comment 36 • 11 years ago

Verified via comment 35.

If needed, we can respin the v1-train build to get this fix included btw.

	Donovan Preston [:fzzzy]
	Comment 37 • 11 years ago

(In reply to Jason Smith [:jsmith] from comment #36)
> Verified via comment 35.
> 
> If needed, we can respin the v1-train build to get this fix included btw.

That might be nice...

	Jason Smith [:jsmith]
	Comment 38 • 11 years ago

(In reply to Donovan Preston from comment #37)
> (In reply to Jason Smith [:jsmith] from comment #36)
> > Verified via comment 35.
> > 
> > If needed, we can respin the v1-train build to get this fix included btw.
> 
> That might be nice...

Okay. I just emailed release engineering asking for a respin.

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/a49ef62295c5

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 11 years ago

(In reply to Jason Smith [:jsmith] from comment #38)
> Okay. I just emailed release engineering asking for a respin.

You can probably trigger these through self-serve yourself, FWIW. Otherwise, any sheriff on IRC certainly can.

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 11 years ago

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

	John Hammink
	Comment 42 • 11 years ago

Verified fixed on

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/282b5c37cf8d
Gaia   e2ef782119b7e79fc62c48d36f0c36909d982988
BuildID 20130712070210
Version 18.0