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

Actually before I nom this, we need to understand the user impact. What's the user impact of this?

(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.

(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.

BTW, this is on m-c only.

(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.

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

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.

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

(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 ?

(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.

(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.

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

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

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

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.

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?

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.

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.

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

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

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.

removing checkin-needed per comment 22

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

(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.

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?

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

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.

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.

#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

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.

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.

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!!

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

(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...

(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.

(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.

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