Closed Bug 1170484 Opened 9 years ago Closed 9 years ago

[e10s] Text correction reverts after correcting a spelling error in Facebook comment

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
40 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
e10s m8+ ---
firefox40 - ---
firefox41 - affected
firefox42 - affected
firefox43 --- fixed

People

(Reporter: akayanni, Assigned: bholley)

References

Details

(Keywords: regression)

Attachments

(4 files, 3 obsolete files)

Cursor error
(deleted), image/png
Details
testcase
(deleted), text/html
Details
Use {Begin,End}PlaceHolderTransaction from mozInlineSpellCheck::ReplaceWord. v1
(deleted), patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Test. v1
(deleted), patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150601075320

Steps to reproduce:

Wrote in a Facebook comment and then corrected a spelling error


Actual results:

After moving the cursor the correction was reverted


Expected results:

Text correction should stick
Issue does NOT happen in safe mode
Issue did NOT exist until the most recent update
(In reply to Yani from comment #1)
> Issue does NOT happen in safe mode
> Issue did NOT exist until the most recent update

WFM with the latest Nightly and the default spelling checker (English).

Are you sure the issue is not with an add-on? (as it works in safe mode)

Could you test with a fresh profile?
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles
Component: Untriaged → Spelling checker
Flags: needinfo?(akayani)
Summary: Text correction reverting → [e10s] Text correction reverting after correcting a spelling error in Facebook comment
Could you test with e10s disabled, please.
Flags: needinfo?(djmach7hn)
Perhaps not with e10s off. Given it isn't totally consistent I'll keep it off for a bit and see if the issue develops.
Flags: needinfo?(akayani)
I can reproduce this bug only with e10s.

Regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f986e55c4e0b&tochange=45a4d6336c73
Summary: [e10s] Text correction reverting after correcting a spelling error in Facebook comment → [e10s] Text correction reverts after correcting a spelling error in Facebook comment
STR from the 1st post are not clear, I repost clear STR:

1) Go to Facebook
2) Type a comment/post and misspell a word.
3) Right-click and select correct/suggested word.
4) Hit enter to send comment/post.

Result:
After hitting enter, post/comment was sent, but with the misspelled word back to it's original form.
Blocks: e10s, CVE-2015-2731
Status: UNCONFIRMED → NEW
tracking-firefox40: --- → ?
tracking-firefox41: --- → ?
Component: Spelling checker → DOM
Ever confirmed: true
Flags: needinfo?(djmach7hn)
Keywords: regression
Version: 41 Branch → 40 Branch
(In reply to Loic from comment #9)
> STR from the 1st post are not clear, I repost clear STR:
> 
> 1) Go to Facebook
> 2) Type a comment/post and misspell a word.
> 3) Right-click and select correct/suggested word.
> 4) Hit enter to send comment/post.
> 
> Result:
> After hitting enter, post/comment was sent, but with the misspelled word
> back to it's original form.

Adding to the STR: if you move the cursor or click outside the text box the corrected word reverts to it original, incorrect form.
Tracking in 41 because regression and 41 is affected.
status-firefox41: --- → affected
tracking-firefox41: ? → +
Tracking enabled in 40 to determine if it is fixed there, as well.
tracking-firefox40: ? → +
Assignee: nobody → gkrizsanits
tracking-e10s: --- → m8+
So is this e10s only?
(In reply to Olli Pettay [:smaug] (for generic DOM reviews, you may want to look for other reviewers too ;)) from comment #13)
> So is this e10s only?

Yes, I verified.
e10s only, not worth tracking for 40 as the aurora cycle is almost over.
tracking-firefox40: + → -
It's absolutely e10s only. I just noticed I've had e10s off for over a week (by mistake) and the issue is back today with it on. I'm dropping plugins starting with Firebug as of now to rule them out.
I am experiencing the same issue - here are the extensions I am running:

Adblock Plus 
BetterPrivacy
Easy Youtube Video Downloader Express
InvisibleHand 

NetVideoHunter
Password Exporter
RightToClick
Self-Destructing Cookies

Tabs on Bottom (Australis)
The Addon Bar (restored)
TinEye Reverse Image Search

WorldIP 
Yahoo Mail Hide Ad Panel
YouTube High Definition
It's not releated to add-ons, you can reproduce it with a fresh profile and the spell checker enabled in Firefox.
just tried the same style of dictionary edit on a message on OK Cupid and that works fine, so this issue may be specific to Facebook.
Agreed, not addons.

There is something else going on that isn't directly related to this. The text cursor which is usually a perfect vertical line develops a 1-2 pixel dag on the upper right. It doesn't seem to be there when you first open the browser but appears sometime later. I've only seen it in Facebook so far. And again only in e10s.
Attached image Cursor error (deleted) — 
This cursor error appears after some amount of text is entered. It may have a relationship to the spell correct issue. They seem to be linked. The spell check works as intended until the cursor develops this dag then that spell check falls on all previous words.
That cursor mark occurs after a soft return to a new line.
Bug 1177686 - (RTL) Mouse cursor has display issues everywhere a text is typed
This patch fixes the bug. But it brings up quite a few questions... How are we going to define when automicrotasks are needed in Gecko? I would add it in evalInSandbox for sure, and probably in nsMessageManagerScriptExecutor::LoadScriptInternal too.
Not sure about the various places where we call getters and setters and might execute scripts. And totally unsure about this case in the patch too, but because of this bug it seems like the right thing to do.
Attachment #8627886 - Flags: feedback?(bugs)
Attachment #8627886 - Flags: feedback?(bobbyholley)
nsMessageManagerScriptExecutor::LoadScriptInternal should be called async, so end of microtask should be anyway at the end of the task.
Comment on attachment 8627886 [details] [diff] [review]
automicrotask in nsFrameMessageManager::ReceiveMessage. v1

Makes sense, though surprising that this fixes the issue.
Do we have several listeners for one message type or what?
Attachment #8627886 - Flags: feedback?(bugs) → feedback+
though, would be safer to push AutoMicroTask even closer to the web apis.
Do you know why the patch fixes the issue?
  

In Gecko whatever web APIs there are with callbacks should, and do deal with microtasks, and for
Gecko internal stuff we shouldn't by default have microtask, for the reasons talked in the other bug.
(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 8627886 [details] [diff] [review]
> automicrotask in nsFrameMessageManager::ReceiveMessage. v1
> 
> Makes sense, though surprising that this fixes the issue.
> Do we have several listeners for one message type or what?

I wish I knew why this fixes the issue. Didn't really wanted to reverse engineer what facebook does, (and us in the chrome code) just found this from adding back mt's one by one from the backed out patch. I don't feel super comfortable about this patch either, do we really want to do microtask every time an nsFrameMessageManager receives a message? With all the CPOWs around that seems like more than we should do.
Blake, do you have any ideas here? I think you worked on the spell checking stuff in e10s.
Flags: needinfo?(mrbkap)
Comment on attachment 8627886 [details] [diff] [review]
automicrotask in nsFrameMessageManager::ReceiveMessage. v1

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

smaug's feedback is sufficient here, though it sounds like he actually wants to move the microtask stuff closer to the APIs that require it.
Attachment #8627886 - Flags: feedback?(bobbyholley) → feedback-
Follow up to the quick chat with Olli over irc, the received messages:
after spellcheck:
-------------------message: InlineSpellChecker:replaceMisspelling
-------------------message: InlineSpellChecker:uninit
after send:
-------------------message: SessionStore:update
-------------------message: ContentPrefs:FunctionCall
-------------------message: ContentPrefs:HandleCompletion
-------------------message: ContentPrefs:FunctionCall
-------------------message: ContentPrefs:HandleCompletion
-------------------message: SessionStore:update

So the parent process sends a replaceMisspelling message and in response the content process version of InlineSpellChecker does some work I guess with the native mozInlineSpellChecker. I guess we don't fire there some events that facebook expects? Or missing something else, that the micro task fixes? I don't know. The editor seems to get the text but whatever else facebook expects to get a notification about the change does not. Or at least that's seem to be the case. In this case I think the microtask stuff is necessary (we do it on the chrome side but then when the work continues on the child side we don't do it there) Olli, do you think we should dive into this further or should I just add the auto microtask a few lines lower and see what treeherder has to say about it?
Flags: needinfo?(bugs)
So what is the smallest piece of code you need to surround with AutoMicrotask to get this fixed?

I don't understand what part of SpellChecker runs in child process and what in parent.
Is RemoteSpellChecker used on parent side? ... I think so.
And replaceWord http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#935 is called on child side?
For debugging could you put AutoMicrotask somewhere there to see if it fixes the issue - 
since I don't understand why we need microtask around message. We should have a microtask end point
when the current runnable has been handled.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #33)
> So what is the smallest piece of code you need to surround with
> AutoMicrotask to get this fixed?
>

Placing it directly into mozInlineSpellChecker::ReplaceWord fixes it.
 
> I don't understand what part of SpellChecker runs in child process and what
> in parent.

On parent side the context menu click happens, we have (I assume) already entered a microtask as we execute script, we get here: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/InlineSpellChecker.jsm#349 and send a message to child here: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/InlineSpellChecker.jsm#552 (and I think we leave microtask on parent side at this point) child process gets the message in nsFrameMessageManager::ReceiveMessage and without entering a micro task on child side we run script: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/InlineSpellCheckerContent.jsm#124 which then calls http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/InlineSpellChecker.jsm#356 and runs mozInlineSpellChecker::ReplaceWord. As much as I can tell at least.

I think we should only enter microtask if sMicroTaskLevel==0. But that's only a gut feeling.

        Attachment #8627886 -
        Attachment is obsolete: true

        Attachment #8629338 -
        Flags: review?(bugs)

    
    

    
  
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #34)
> Placing it directly into mozInlineSpellChecker::ReplaceWord fixes it.
Did you play there to see where exactly AutoMicrotask needs to be.
Would be good to know why we need the automicrotask.

> I think we should only enter microtask if sMicroTaskLevel==0. But that's
> only a gut feeling.
Well, the callbacks run when level==0 again, so doesn't matter

    
    

    
  
Comment on attachment 8629338 [details] [diff] [review]
automicrotask in nsFrameMessageManager::ReceiveMessage. v2

I think we either need to change how e10s spellchecker communication happens so that end-of-task microtask-end-point works here, or push the AutoMicrotask closer to the place where we touch DOM.

        Attachment #8629338 -
        Flags: review?(bugs) → review-

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #38)
> Comment on attachment 8629338 [details] [diff] [review]
> automicrotask in nsFrameMessageManager::ReceiveMessage. v2
> 
> I think we either need to change how e10s spellchecker communication happens
> so that end-of-task microtask-end-point works here, or push the
> AutoMicrotask closer to the place where we touch DOM.

How would that solve the problem? In my mind the problem is that an event on parent side (content menu click in this case) runs script on the child side without entering a microtask. Trying to fix this single case does not fix the conceptual problem we have here (and which can cause countless similar issues) that the current micro task model is just not e10s compatible.

Without e10s we do:
#29 0x00007ffff00d856a in PresShell::HandleDOMEventWithTarget (
---Type <return> to continue, or q <return> to quit---
    this=0x7fffc70e9400, aTargetContent=0x7fffb9c1a430, aEvent=0x7fffb286f110, 
    aStatus=0x7fffffffbfd4)
    at /home/gabor/development/mozilla-central/layout/base/nsPresShell.cpp:8155
#30 0x00007fffee11e404 in nsContentUtils::DispatchXULCommand (
    aTarget=0x7fffb9c1a430, aTrusted=true, aSourceEvent=0x0, 
    aShell=0x7fffc70e9400, aCtrl=false, aAlt=false, aShift=false, aMeta=false)
    at /home/gabor/development/mozilla-central/dom/base/nsContentUtils.cpp:6063
#31 0x00007ffff0323b4c in nsXULMenuCommandEvent::Run (this=0x7fffbc9f7850)
    at /home/gabor/development/mozilla-central/layout/xul/nsXULPopupManager.cpp:2622

Because of the DOM event I would assume everything that is microtask checkpoint related (DOM mutation events / promisses) are taken care of. But with e10s in the child process we have none of this.
There we just call into a JS function (in a jsm) that can just manipulate the content DOM (without DOM mutation could work) or do some promises which I would doubt would work as they should since we don't seem to call Promise::PerformMicroTaskCheckpoint anywhere either.

> Did you play there to see where exactly AutoMicrotask needs to be.
> Would be good to know why we need the automicrotask.

Well, since mozInlineSpellChecker::ReplaceWord does a transaction, I don't think it matters much where I put it. But I assume it's the DOM mutation event what is broken without AutoMicrotask. (http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#5227)

> > I think we should only enter microtask if sMicroTaskLevel==0. But that's
> > only a gut feeling.
> Well, the callbacks run when level==0 again, so doesn't matter

I'm not sure. What is the relationship between the IPC message pump and callbacks? If a jsm function on child side within a callback fires a sync message to the parent, that seems to be receiving the response in this method as well, and then the level!=0.

Either way I can just place the microtask in the mozInlineSpellChecker but that might come with side effects and don't fixes the real issue just this bug.

Or can try to send a sync message from the parent to the child when the context menu click happens, but that would block chrome which is a bad practice in itself, and even more importantly would not transfer the microtask from parent to child, so it would not fix the problem.

Is it stupid to do HandleDOMEventWithTarget like DispatchXULCommand instead of just calling directly into the JS function in the async case at least?
Flags: needinfo?(bugs)

    
    

    
  
This problem seems exclusive to Facebook only, every other place I have tried using the spelling correction works just fine.

    
    

    
  
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #39)
> How would that solve the problem? In my mind the problem is that an event on
> parent side (content menu click in this case) runs script on the child side
> without entering a microtask.
We're in a task, so we are also in a microtask.

> Because of the DOM event I would assume everything that is microtask
> checkpoint related (DOM mutation events / promisses) are taken care of. But
> with e10s in the child process we have none of this.
Dispatching DOM Event doesn't mean entering to a microtask. It is calling some JS callback which enters microtask.

> I'm not sure. What is the relationship between the IPC message pump and
> callbacks? If a jsm function on child side within a callback fires a sync
> message to the parent, that seems to be receiving the response in this
> method as well, and then the level!=0.
So?



> Either way I can just place the microtask in the mozInlineSpellChecker but
> that might come with side effects and don't fixes the real issue just this
> bug.
Well I'd like to understand what is causing the bug, not just putting random AutoMicrotask somewhere.


> Is it stupid to do HandleDOMEventWithTarget like DispatchXULCommand instead
> of just calling directly into the JS function in the async case at least?
Don't understand this.
Flags: needinfo?(bugs)

    
    

    
  
Remember that there is http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#996
End of microtask is end of outermost script execution (from web page point of view) or end of a task.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #41)
> We're in a task, so we are also in a microtask.

Could you show me where do we enter a microtask? This is the stack in the child in e10s:

e10s child process:
===================

(gdb) backtrace
#0  mozInlineSpellChecker::ReplaceWord (this=0x7fffcfab4df0, 
    aNode=0x7fffcb7c4a50, aOffset=3, newword=...)
    at /home/gabor/development/mozilla-central/extensions/spellcheck/src/mozInlineSpellChecker.cpp:938
#1  0x00007fffeef16c93 in NS_InvokeByIndex (that=0x7fffcfab4df0, 
    methodIndex=11, paramCount=3, params=0x7fffffff6ce8)
    at /home/gabor/development/mozilla-central/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:176
#2  0x00007fffefb888a7 in CallMethodHelper::Invoke (this=0x7fffffff6ca0)
    at /home/gabor/development/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:2080
#3  0x00007fffefb865c3 in CallMethodHelper::Call (this=0x7fffffff6ca0)
    at /home/gabor/development/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1417
#4  0x00007fffefb6b860 in XPCWrappedNative::CallMethod (ccx=..., 
    mode=XPCWrappedNative::CALL_METHOD)
    at /home/gabor/development/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:1384
#5  0x00007fffefb74c35 in XPC_WN_CallMethod (cx=0x7fffe4f95840, argc=3, 
    vp=0x7fffdd282118)
    at /home/gabor/development/mozilla-central/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1144
#6  0x00007ffff3c95cf5 in js::CallJSNative (cx=0x7fffe4f95840, 
---Type <return> to continue, or q <return> to quit---
    native=0x7fffefb748d5 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/gabor/development/mozilla-central/js/src/jscntxtinlines.h:235
#7  0x00007ffff3c658e2 in js::Invoke (cx=0x7fffe4f95840, args=..., 
    construct=js::NO_CONSTRUCT)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:711
#8  0x00007ffff3c74b13 in Interpret (cx=0x7fffe4f95840, state=...)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:2959
#9  0x00007ffff3c6550f in js::RunScript (cx=0x7fffe4f95840, state=...)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:655
#10 0x00007ffff3c65a23 in js::Invoke (cx=0x7fffe4f95840, args=..., 
    construct=js::NO_CONSTRUCT)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:731
#11 0x00007ffff3c65df3 in js::Invoke (cx=0x7fffe4f95840, thisv=..., fval=..., 
    argc=1, argv=0x7fffdd2820a0, rval=...)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:768
#12 0x00007ffff42f2d65 in js::DirectProxyHandler::call (
    this=0x7ffff7803a40 <js::CrossCompartmentWrapper::singleton>, 
    cx=0x7fffe4f95840, proxy=..., args=...)
    at /home/gabor/development/mozilla-central/js/src/proxy/DirectProxyHandler.cpp:77
#13 0x00007ffff42f03e9 in js::CrossCompartmentWrapper::call (
    this=0x7ffff7803a40 <js::CrossCompartmentWrapper::singleton>, 
---Type <return> to continue, or q <return> to quit---
    cx=0x7fffe4f95840, wrapper=..., args=...)
    at /home/gabor/development/mozilla-central/js/src/proxy/CrossCompartmentWrapper.cpp:289
#14 0x00007ffff42f6577 in js::Proxy::call (cx=0x7fffe4f95840, proxy=..., 
    args=...)
    at /home/gabor/development/mozilla-central/js/src/proxy/Proxy.cpp:391
#15 0x00007ffff42f79ec in js::proxy_Call (cx=0x7fffe4f95840, argc=1, 
    vp=0x7fffdd282090)
    at /home/gabor/development/mozilla-central/js/src/proxy/Proxy.cpp:697
#16 0x00007ffff3c95cf5 in js::CallJSNative (cx=0x7fffe4f95840, 
    native=0x7ffff42f790a <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/gabor/development/mozilla-central/js/src/jscntxtinlines.h:235
#17 0x00007ffff3c657e0 in js::Invoke (cx=0x7fffe4f95840, args=..., 
    construct=js::NO_CONSTRUCT)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:699
#18 0x00007ffff3c74b13 in Interpret (cx=0x7fffe4f95840, state=...)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:2959
#19 0x00007ffff3c6550f in js::RunScript (cx=0x7fffe4f95840, state=...)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:655
#20 0x00007ffff3c65a23 in js::Invoke (cx=0x7fffe4f95840, args=..., 
    construct=js::NO_CONSTRUCT)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:731
---Type <return> to continue, or q <return> to quit---
#21 0x00007ffff3c65df3 in js::Invoke (cx=0x7fffe4f95840, thisv=..., fval=..., 
    argc=1, argv=0x7fffffffb008, rval=...)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:768
#22 0x00007ffff42f2d65 in js::DirectProxyHandler::call (
    this=0x7ffff7803a40 <js::CrossCompartmentWrapper::singleton>, 
    cx=0x7fffe4f95840, proxy=..., args=...)
    at /home/gabor/development/mozilla-central/js/src/proxy/DirectProxyHandler.cpp:77
#23 0x00007ffff42f03e9 in js::CrossCompartmentWrapper::call (
    this=0x7ffff7803a40 <js::CrossCompartmentWrapper::singleton>, 
    cx=0x7fffe4f95840, wrapper=..., args=...)
    at /home/gabor/development/mozilla-central/js/src/proxy/CrossCompartmentWrapper.cpp:289
#24 0x00007ffff42f6577 in js::Proxy::call (cx=0x7fffe4f95840, proxy=..., 
    args=...)
    at /home/gabor/development/mozilla-central/js/src/proxy/Proxy.cpp:391
#25 0x00007ffff42f79ec in js::proxy_Call (cx=0x7fffe4f95840, argc=1, 
    vp=0x7fffffffaff8)
    at /home/gabor/development/mozilla-central/js/src/proxy/Proxy.cpp:697
#26 0x00007ffff3c95cf5 in js::CallJSNative (cx=0x7fffe4f95840, 
    native=0x7ffff42f790a <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/gabor/development/mozilla-central/js/src/jscntxtinlines.h:235
---Type <return> to continue, or q <return> to quit---
#27 0x00007ffff3c657e0 in js::Invoke (cx=0x7fffe4f95840, args=..., 
    construct=js::NO_CONSTRUCT)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:699
#28 0x00007ffff3c65df3 in js::Invoke (cx=0x7fffe4f95840, thisv=..., fval=..., 
    argc=1, argv=0x7fffffffb4d0, rval=...)
    at /home/gabor/development/mozilla-central/js/src/vm/Interpreter.cpp:768
#29 0x00007ffff41d292b in JS_CallFunctionValue (cx=0x7fffe4f95840, obj=..., 
    fval=..., args=..., rval=...)
    at /home/gabor/development/mozilla-central/js/src/jsapi.cpp:4567
#30 0x00007ffff0245b9e in nsFrameMessageManager::ReceiveMessage (
    this=0x7fffdcefc870, aTarget=0x7fffdcfe5b00, aTargetFrameLoader=0x0, 
    aTargetClosed=false, aMessage=..., aIsSync=false, 
    aCloneData=0x7fffffffb750, aCpows=0x7fffffffb770, aPrincipal=0x0, 
    aRetVal=0x0)
    at /home/gabor/development/mozilla-central/dom/base/nsFrameMessageManager.cpp:1250
#31 0x00007ffff0244433 in nsFrameMessageManager::ReceiveMessage (
    this=0x7fffdcefc870, aTarget=0x7fffdcfe5b00, aTargetFrameLoader=0x0, 
    aMessage=..., aIsSync=false, aCloneData=0x7fffffffb750, 
    aCpows=0x7fffffffb770, aPrincipal=0x0, aRetVal=0x0)
    at /home/gabor/development/mozilla-central/dom/base/nsFrameMessageManager.cpp:1067
#32 0x00007ffff1a82ed0 in mozilla::dom::TabChild::RecvAsyncMessage(nsString cons---Type <return> to continue, or q <return> to quit---
t&, mozilla::dom::ClonedMessageData const&, nsTArray<mozilla::jsipc::CpowEntry>&&, IPC::Principal const&) (this=0x7fffd732e800, aMessage=..., aData=..., 
    aCpows=<unknown type in /home/gabor/development/mozilla-central/obj-x86_64-unknown-linux-gnu/dist/bin/libxul.so, CU 0xb64486d, DIE 0xb792242>, 
    aPrincipal=...)
    at /home/gabor/development/mozilla-central/dom/ipc/TabChild.cpp:2741
#33 0x00007fffef82bc85 in mozilla::dom::PBrowserChild::OnMessageReceived (
    this=0x7fffd732e9a0, msg__=...)
    at /home/gabor/development/mozilla-central/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PBrowserChild.cpp:2512
#34 0x00007fffef90e797 in mozilla::dom::PContentChild::OnMessageReceived (
    this=0x7fffe4f1d030, msg__=...)
    at /home/gabor/development/mozilla-central/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PContentChild.cpp:5435
#35 0x00007fffef41e23b in mozilla::ipc::MessageChannel::DispatchAsyncMessage (
    this=0x7fffe4f1d098, aMsg=...)
    at /home/gabor/development/mozilla-central/ipc/glue/MessageChannel.cpp:1279
#36 0x00007fffef41dce7 in mozilla::ipc::MessageChannel::DispatchMessage (
    this=0x7fffe4f1d098, aMsg=...)
    at /home/gabor/development/mozilla-central/ipc/glue/MessageChannel.cpp:1198
#37 0x00007fffef41dbde in mozilla::ipc::MessageChannel::OnMaybeDequeueOne (
    this=0x7fffe4f1d098)
    at /home/gabor/development/mozilla-central/ipc/glue/MessageChannel.cpp:1182
---Type <return> to continue, or q <return> to quit---
#38 0x00007fffef4333be in DispatchToMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()> (obj=0x7fffe4f1d098, 
    method=(bool (mozilla::ipc::MessageChannel::*)(mozilla::ipc::MessageChannel * const)) 0x7fffef41da00 <mozilla::ipc::MessageChannel::OnMaybeDequeueOne()>, 
    arg=...)
    at /home/gabor/development/mozilla-central/ipc/chromium/src/base/tuple.h:387
#39 0x00007fffef432e3e in RunnableMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)(), Tuple0>::Run (this=0x7fffe4f07840)
    at /home/gabor/development/mozilla-central/ipc/chromium/src/base/task.h:310
#40 0x00007fffef42698b in mozilla::ipc::MessageChannel::RefCountedTask::Run (
    this=0x7fffe4f506c0) at ../../dist/include/mozilla/ipc/MessageChannel.h:446
#41 0x00007fffef426b60 in mozilla::ipc::MessageChannel::DequeueTask::Run (
    this=0x7fffcab23ce0) at ../../dist/include/mozilla/ipc/MessageChannel.h:463
#42 0x00007fffef3b262d in MessageLoop::RunTask (this=0x7fffffffd700, 
    task=0x7fffcab23ce0)
    at /home/gabor/development/mozilla-central/ipc/chromium/src/base/message_loop.cc:364
#43 0x00007fffef3b26a5 in MessageLoop::DeferOrRunPendingTask (
    this=0x7fffffffd700, pending_task=...)
    at /home/gabor/development/mozilla-central/ipc/chromium/src/base/message_loop.cc:372

    
    

    
  
bug 1180686 explains where to bug is.

    
    

    
  
I'm having trouble understanding what's going on here. It seems like maybe Facebook has a mutation event listener somewhere and it's not getting invoked when we change the DOM in ReplaceWord. Is that right? If so, would it fix the problem to make this code run in a different way?
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/InlineSpellChecker.jsm#356
Maybe we could put run it via Services.tm.mainThread.dispatch?

That might be a better short-term fix for this until bug 1180686 is fixed.

    
    

    
  
(In reply to Bill McCloskey (:billm) from comment #45)
> That might be a better short-term fix for this until bug 1180686 is fixed.

Let's see how that bug progresses. I think that one is just as important/urgent as this one, and we could use this one as an indicator/test. But I will try to write up a simpler testcase in the meanwhile here. Anyway, if bug 1180686 goes slower than it should we can go with your quickfix ofc.

    
    

    
  
Since the other bug was rated as + and this is m8, also that one will likely take more time to get somewhere, I'm going to try my luck with the workaround in comment 45 tomorrow.

    
    

    
  
Taking this.
Assignee: gkrizsanits → bobbyholley
Flags: needinfo?(mrbkap)

    
    

    
  

      
      
      
      

        Attached file
          
        testcase (deleted)
        — 
      

    


  
The facebook code is react-y. It looks like they're probably using mutation observers to grab a copy of the text for their in-memory model, and then restoring stale data at some point down the line.

I set a breakpoint in their mutation listener and compared the behavior between e10s and non-e10s. It looks like there are two mutations, which are combined (in an array) in non-e10s, and delivered in two separate calls on e10s.

Here's a reduced testcase. I'll have a look tomorrow and see where the difference is coming from.

    
    

    
  
khuey is fixing this by making ipc message/runnable working like normal runnables.
The issue is that currently ipc messages aren't treated as normal xpcom runnables (several ipc messages may be processed during one xpcom runnable), so 
AfterProcessNextEvent and other callbacks aren't called when they should be.

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #50)
> Here's a reduced testcase. I'll have a look tomorrow and see where the
> difference is coming from.

It comes from here: https://bugzilla.mozilla.org/show_bug.cgi?id=1180686#c8

(In reply to Olli Pettay [:smaug] from comment #51)
> khuey is fixing this by making ipc message/runnable working like normal
> runnables.
> The issue is that currently ipc messages aren't treated as normal xpcom
> runnables (several ipc messages may be processed during one xpcom runnable),
> so 
> AfterProcessNextEvent and other callbacks aren't called when they should be.

I don't think his fix will address the issue Bill pointed out. But it fixes something
related... because with khuey's patch and using a setTimeout on content side in the inline
spellchecker it seems to work. (before his patch that workaround did not work, but maybe
only because I did something wrong since it _should_ work...)

Anyway, I think it's important to fix this bug properly, but in the meanwhile if that takes
too much time we can go with the setTimeout workaround...

    
    

    
  
Kyle, what's your timeline on bug 1179909? Should I dive in now, or do other things while you're finishing up with it?
Flags: needinfo?(khuey)

    
    

    
  
I'm working through test failures.  There's only remaining issue that I'm aware of, although its non-trivial to fix.
Flags: needinfo?(khuey)

    
    

    
  
So in the e10s case, correcting a spellchecking causes us to trigger mutation observers twice (see comment 50).

First here: https://pastebin.mozilla.org/8843146 , then here: https://pastebin.mozilla.org/8843147

The key point here is that mozInlineSpellChecker::ReplaceWord first does DeleteSelection and then InsertText on the editor, which both instantiate separate nsAutoPlaceHolderBatch guards, which end up trigger mutation observers.

In the non-e10s case, we end up triggering the mutation observer here: https://pastebin.mozilla.org/8843148

With no mozInlineSpellChecker on the stack.

Commenting out the nsAutoPlaceHolderBatch in DeleteSelection fixes the facebook issue for me, so it looks like I was correct in associating the reduced behavioral difference here with the user-visible behavioral difference on facebook.

I need to head out, but I'll have a closer look at the stacks sometime later and see what's causing the difference.

    
    

    
  
Untracking for FF41 as e10s is not enabled by default. Tracked for FF42 instead.

          status-firefox42:
          --- → ?

          tracking-firefox41:
          + → -

          tracking-firefox42:
          --- → +

    
    

    
  
Getting back to this, and continuing from comment 55.

The reason for the behavioral difference between e10s and non-e10s is that the non-e10s case has some chrome code on the stack (see the stack in comment 55), and that code does a microtask checkpoint. So the reason why it works in Release is pretty incidental.

As far as I can tell, Gecko's behavior isn't buggy here per se - the decision to notify mutation observers twice (once for the removal of the old text, and once for the insertion of the replacement text) or once (as a batched update) shouldn't affect the correctness of the page. Both could theoretically be performed by the page, the user, or an addon. The issue seems to be that the Facebook code triggered by the mutation observer doesn't properly handle the delivery of multiple mutation observer notifications before returning to the event loop. This bug could probably surface in other ways, and they probably want to fix it. I'll email some folks to get this to the right people.

Meanwhile, we can easily fix this in Gecko by doing a Microtask checkpoint around the smallest set of operations that we want to be observable to script. I can fix this by adding an nsAutoMicroTask in mozInlineSpellChecker::ReplaceWord, but I think it would make even more sense to piggy-back this on top of editor transactions. I'll work up a patch to do that and see if it breaks anything.

    
    

    
  


  

        Attachment #8650209 -
        Flags: review?(ehsan)

    
    

    
  
Ehsan, if you're not up to speed on the current microtask stuff feel free to bounce to khuey.

    
    

    
  
Update from Facebook - they're fixing this, and should be in production next week, irrespective of the Gecko change here (which we should nonetheless make).

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #60)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=36f2d46ca339

Looks green.

    
  
Depends on: 1196638

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #59)
> Ehsan, if you're not up to speed on the current microtask stuff feel free to
> bounce to khuey.

I'm not, but I would like to understand what you're trying to do here, and why this is a correct patch, since our test coverage on the editor is abysmal.

What's the gist of the issue here?  This bug is extremely difficult to follow, but I think comment 55 and 57 describe the latest findings?  What I don't understand is why is an editor transaction the right place for this to go in?

More specifically, you realize that nsEditor::BeginTransaction is almost never called for an editor transaction, right?  :-)  I think you're just relying on a quirk in mozInlineSpellChecker::ReplaceWord() (I have no idea why that code calls nsEditor::Begin/EndTransaction!) and therefore are adding a hack which may stop to work any time...

    
    

    
  
Comment on attachment 8650209 [details] [diff] [review]
Enter a MicroTask during Editor transactions. v1

r- for now since this is almost definitely the wrong place to enter a microtask.

        Attachment #8650209 -
        Flags: review?(ehsan) → review-

    
    

    
  
(In reply to Ehsan Akhgari (don't ask for review please) from comment #63)
> What's the gist of the issue here?  This bug is extremely difficult to
> follow, but I think comment 55 and 57 describe the latest findings?

Correct.

> What I don't understand is why is an editor transaction the right place for this to
> go in?

Naively: because we're trying to avoid exposing intermediate editor states to mutation observers, and editor transactions are documented as "a signal from the caller to the editor that the caller will execute multiple updates to the content tree that should be treated as a single logical operation" (nsIEditor.idl).
 
> More specifically, you realize that nsEditor::BeginTransaction is almost
> never called for an editor transaction, right?  :-)

I don't know what would cause me to realize that. Certainly not the documentation. ;-)

> I think you're just
> relying on a quirk in mozInlineSpellChecker::ReplaceWord() (I have no idea
> why that code calls nsEditor::Begin/EndTransaction!)

Presumably because it's trying to indicate that these multiple updates should be treated as a single logical operation?

> and therefore are adding a hack which may stop to work any time...

From the perspective of an outsider, that doesn't seem like a fair assessment. We want to scope a microtask checkpoint around logically atomic editor operations, such that, when they're performed by native code without script on the stack, we don't notify mutation observers of intermediate states. We could put this directly in the spellchecker, but it seems better to make it more general, and editor transactions _appear_ to be the appropriate level abstraction to do that.

It may well be that editor transactions are not the abstraction they claim to me, or that we want to move away from them for some reason, which is your call. But I want to you to understand why this patch appears to me to be the right thing to do, so that hopefully you can suggest something more appropriate. :-)
Flags: needinfo?(ehsan)

    
    

    
  
smaug makes the great point on IRC that we probably _also_ don't want to dispatch input events mid-way through operation, and a microtask checkpoint wouldn't help us there. So it seems like we also want to defer input event firing until the end of the "transaction".

Ehsan, if not {Begin,End}Transaction, what machinery do you want to use to represent this transaction-in-quotes?

    
    

    
  
...and just dispatching the latter input event would move end-of-microtask check point later, and we would have only one such, if I interpret the e10s stacks correctly.

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #65)
> > What I don't understand is why is an editor transaction the right place for this to
> > go in?
> 
> Naively: because we're trying to avoid exposing intermediate editor states
> to mutation observers, and editor transactions are documented as "a signal
> from the caller to the editor that the caller will execute multiple updates
> to the content tree that should be treated as a single logical operation"
> (nsIEditor.idl).

Does this mean that we want to do this for *all* editor transactions?  Or just for replacing a misspelled word?  Your patch does the latter.

> > More specifically, you realize that nsEditor::BeginTransaction is almost
> > never called for an editor transaction, right?  :-)
> 
> I don't know what would cause me to realize that. Certainly not the
> documentation. ;-)

Heh, not blaming you.  For future reference, many of the names used in editor/ are just lies.  For example, BeginTransaction doesn't really mean begin a transaction...  I'm sorry for the extremely sad state of things in this code :(

> > I think you're just
> > relying on a quirk in mozInlineSpellChecker::ReplaceWord() (I have no idea
> > why that code calls nsEditor::Begin/EndTransaction!)
> 
> Presumably because it's trying to indicate that these multiple updates
> should be treated as a single logical operation?

So in editor, we have the idea of editing transactions.  Those are basically editor/libeditor/*Txn.*.  There is no transaction for replacing a string but you can also join transactions together.  It seems like nsEditor::BeginTransaction bypasses this setup...   Anyways, the point I want to get to is the behavior that we want.  I think from what you are describing, we want to do this for all editor transactions?

> > and therefore are adding a hack which may stop to work any time...
> 
> From the perspective of an outsider, that doesn't seem like a fair
> assessment. We want to scope a microtask checkpoint around logically atomic
> editor operations, such that, when they're performed by native code without
> script on the stack, we don't notify mutation observers of intermediate
> states. We could put this directly in the spellchecker, but it seems better
> to make it more general, and editor transactions _appear_ to be the
> appropriate level abstraction to do that.

I agree!

> It may well be that editor transactions are not the abstraction they claim
> to me, or that we want to move away from them for some reason, which is your
> call. But I want to you to understand why this patch appears to me to be the
> right thing to do, so that hopefully you can suggest something more
> appropriate. :-)

I think you should move this to nsTransactionManager::Begin/EndTransaction, which is the thing that gets called for all editor transactions including this one.  But since we use nsTransactionManager for the UndoManager API as well, we need to decide whether we want this behavior for UndoManager as well.  From what you are describing, I think the answer is no.  If you agree, you should add a boolean member to nsTransactionManager to differentiate whether it's instantiated for editor vs something else, and make this behavior depend on the boolean flag.

Also, please add a test case for this.

(In reply to Bobby Holley (:bholley) from comment #66)
> smaug makes the great point on IRC that we probably _also_ don't want to
> dispatch input events mid-way through operation, and a microtask checkpoint
> wouldn't help us there. So it seems like we also want to defer input event
> firing until the end of the "transaction".

We dispatch input events off of EndPlaceHolderTransaction which is hooked up to nsAutoPlaceHolderBatch.  This should only trigger when the IME composition is finished which happens after the user finishes typing something in.  That is separate from the editor transaction management infrastructure.  Double checking through the code, it appears to me that we don't fire the input event midway like this.
Flags: needinfo?(ehsan)

    
    

    
  
(In reply to Ehsan Akhgari (don't ask for review please) from comment #68)
> (In reply to Bobby Holley (:bholley) from comment #65)

> Does this mean that we want to do this for *all* editor transactions?  Or
> just for replacing a misspelled word?  Your patch does the latter.

The former, I think.

> Heh, not blaming you.  For future reference, many of the names used in
> editor/ are just lies.  For example, BeginTransaction doesn't really mean
> begin a transaction...  I'm sorry for the extremely sad state of things in
> this code :(

So it goes in Gecko - I'm sure it used to be much worse before you started owning it! :-)

> It seems like nsEditor::BeginTransaction bypasses this setup

Does that mean I need it in both nsEditor::BT and nsTrasactionManager::BT? Or am I misinterpreting 'bypasses'?

> I think you should move this to nsTransactionManager::Begin/EndTransaction,
> which is the thing that gets called for all editor transactions including
> this one.

Makes sense.

> But since we use nsTransactionManager for the UndoManager API as
> well, we need to decide whether we want this behavior for UndoManager as
> well.  From what you are describing, I think the answer is no.

Can you explain this more? What is the transaction granularity for undo? Naively, I think we'd want one mutation observer per atomic undo unit.

> Also, please add a test case for this.

Wrote one already, but for the record, it was _hard_, and probably would have been impossible for someone without intimate knowledge of our codebase. In retrospect, I think it probably wasn't worth the time spent (which is rare).

> (In reply to Bobby Holley (:bholley) from comment #66)
> > smaug makes the great point on IRC that we probably _also_ don't want to
> > dispatch input events mid-way through operation, and a microtask checkpoint
> > wouldn't help us there. So it seems like we also want to defer input event
> > firing until the end of the "transaction".
> 
> We dispatch input events off of EndPlaceHolderTransaction which is hooked up
> to nsAutoPlaceHolderBatch.  This should only trigger when the IME
> composition is finished which happens after the user finishes typing
> something in.  That is separate from the editor transaction management
> infrastructure.  Double checking through the code, it appears to me that we
> don't fire the input event midway like this.

Not sure what you mean - the stacks in comment 55 clearly show us firing midway input events...
Flags: needinfo?(ehsan)

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #69)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #68)
> > (In reply to Bobby Holley (:bholley) from comment #65)
> 
> > Does this mean that we want to do this for *all* editor transactions?  Or
> > just for replacing a misspelled word?  Your patch does the latter.
> 
> The former, I think.

OK, good.  This is the main issue that was confusing me!

> > It seems like nsEditor::BeginTransaction bypasses this setup
> 
> Does that mean I need it in both nsEditor::BT and nsTrasactionManager::BT?
> Or am I misinterpreting 'bypasses'?

I meant it bypasses doing things "properly", but it will end up calling nsTransactionManager::BeginTransaction, so you won't need the calls in both places, only in nsTransactionManager.

> > But since we use nsTransactionManager for the UndoManager API as
> > well, we need to decide whether we want this behavior for UndoManager as
> > well.  From what you are describing, I think the answer is no.
> 
> Can you explain this more? What is the transaction granularity for undo?
> Naively, I think we'd want one mutation observer per atomic undo unit.

This is the UndoManager API I'm talking about: <https://dvcs.w3.org/hg/undomanager/raw-file/tip/undomanager.html>.  We have an implementation of it that is disabled.  It uses nsTransactionManager: <http://mxr.mozilla.org/mozilla-central/source/dom/html/UndoManager.cpp#841>

nsTransactionManager is a generic transaction manager that really doesn't have much to do with the editor.  It has the concept of "transactions" which are things that implement nsITransaction: <http://mxr.mozilla.org/mozilla-central/source/editor/txmgr/nsITransaction.idl#15>.  Those things know how to do, undo, and redo themselves, and they also know how to join other transactions, which things such as EditAggregateTxn use <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/EditAggregateTxn.cpp#74> in order to combine several transactions into one.  So the granularity of the transaction is adjustable at runtime.

I hope this answers your question...

> > Also, please add a test case for this.
> 
> Wrote one already, but for the record, it was _hard_, and probably would
> have been impossible for someone without intimate knowledge of our codebase.
> In retrospect, I think it probably wasn't worth the time spent (which is
> rare).

Ugh.  :(  But think of the bright side.  This stuff is all glued to each other using saliva, and it's only tests that keep things from crumbling down, especially stuff this complicated.  :-)

> > (In reply to Bobby Holley (:bholley) from comment #66)
> > > smaug makes the great point on IRC that we probably _also_ don't want to
> > > dispatch input events mid-way through operation, and a microtask checkpoint
> > > wouldn't help us there. So it seems like we also want to defer input event
> > > firing until the end of the "transaction".
> > 
> > We dispatch input events off of EndPlaceHolderTransaction which is hooked up
> > to nsAutoPlaceHolderBatch.  This should only trigger when the IME
> > composition is finished which happens after the user finishes typing
> > something in.  That is separate from the editor transaction management
> > infrastructure.  Double checking through the code, it appears to me that we
> > don't fire the input event midway like this.
> 
> Not sure what you mean - the stacks in comment 55 clearly show us firing
> midway input events...

Ah crap, of course :(  It's because we have this here: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsPlaintextEditor.cpp#659>

So according to the spec, we're supposed to fire the input event off of a task.  Maybe we should just do that?
Flags: needinfo?(ehsan)

    
    

    
  
firing input event using a task sounds like a super regression prone change.
I'd rather just suppress the first input event, or effectively dispatch one input event
(using a script runner) once the we've done the "delete-insert"

    
    

    
  
If Facebook fixed this on their side, we should just close this bug as WFM, or at least re-triage it since it's definitely no longer m8.

For microtask magic it would be enough to do the microtask in mozInlineSpellChecker::ReplaceWord (block should go around DeleteSelection and InsertText calls), but I personally feel quite strongly against it after talking to Olli about it a few months ago. It feels like a foot-gun to do a microtask checkpoint at a place like that.

I don't see a good reason why would we need a hack for this right now, but if we need one locally, we can just call the spell checker in a setTimeout call for e10s, that would workaround the special behavior e10s has for microtask checkpoints, and would not introduce new weird bugs.

If we want to handle the way e10s does microtask checkpoints (doing it only after a batch of chrome tasks instead of after every single one separately) we should do that in a separate bug.

    
    

    
  
well, there is definitely the bug of firing two input events, and that is in fact, based on the stacks, the cause of this bug on our side. And that is a bug to fix.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #73)
> well, there is definitely the bug of firing two input events, and that is in
> fact, based on the stacks, the cause of this bug on our side. And that is a
> bug to fix.

I mean if on Facebook it is not reproducible, the issue should go into a new bug or we should at least stop tracking it as an e10s uplift blocker. But I've just tested it and the Facebook fix is not released yet, so nevermind.

    
    

    
  
We don't know that this only breaks Facebook.

(In reply to Olli Pettay [:smaug] from comment #71)
> firing input event using a task sounds like a super regression prone change.

Agreed.

> I'd rather just suppress the first input event, or effectively dispatch one
> input event
> (using a script runner) once the we've done the "delete-insert"

That sounds fine to me.

    
    

    
  


  
Both InsertText and DeleteSelection do a placeholder transaction. When the placeholder
transaction depth drops to zero, we fire input events (which in turn does a microtask
checkpoint). So we can prevent that from happening mid-operation by scoping a larger
placeholder transaction around both calls.

        Attachment #8629338 -
        Attachment is obsolete: true

        Attachment #8650209 -
        Attachment is obsolete: true

        Attachment #8651329 -
        Flags: review?(ehsan)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Test. v1 (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #8651330 -
        Flags: review?(ehsan)

    
    

    
  
Comment on attachment 8651329 [details] [diff] [review]
Use {Begin,End}PlaceHolderTransaction from mozInlineSpellCheck::ReplaceWord. v1

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

::: extensions/spellcheck/src/mozInlineSpellChecker.cpp
@@ +949,4 @@
>    
>      nsCOMPtr<nsISelection> selection;
>      res = editor->GetSelection(getter_AddRefs(selection));
>      NS_ENSURE_SUCCESS(res, res);

Please use nsAutoPlaceHolderBatch instead of calling these manually so that we do the right thing in the early return here.

        Attachment #8651329 -
        Flags: review?(ehsan) → review+

    
  

        Attachment #8651330 -
        Flags: review?(ehsan) → review+

    
    

    
  
For future reference, mfbt/GuardObjects.h contains helper macros that would catch mistakes like this.

    
    

    
  
(In reply to Ehsan Akhgari (don't ask for review please) from comment #84)
> For future reference, mfbt/GuardObjects.h contains helper macros that would
> catch mistakes like this.

Yes, it's too bad that the editor guards don't use them! I would have added them, but it was already a bustage fix on inbound, so I didn't want to add more things.

Can't we have some sort of static analysis that bases this on MOZ_STACK_CLASS or something?

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #85)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #84)
> > For future reference, mfbt/GuardObjects.h contains helper macros that would
> > catch mistakes like this.
> 
> Yes, it's too bad that the editor guards don't use them! I would have added
> them, but it was already a bustage fix on inbound, so I didn't want to add
> more things.

Fair, submitted a patch in bug 1198385.

> Can't we have some sort of static analysis that bases this on
> MOZ_STACK_CLASS or something?

Not all MOZ_STACK_CLASSes are guard objects...  :/
See Also:  → https://bugzilla.mozilla.org/show_bug.cgi?id=1198385

    
    

    
  
(In reply to Ehsan Akhgari (don't ask for review please) from comment #86)
> Not all MOZ_STACK_CLASSes are guard objects...  :/

Yes but I'd think most of them are (or am I wrong here?). Can't we have an opt-out annotation for the ones that aren't, similar to MOZ_IMPLICIT? i.e. MOZ_NONGUARD_STACK_CLASS

The guard object helpers are really ugly, FWIW. I wish we could just do that statically.

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #87)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #86)
> > Not all MOZ_STACK_CLASSes are guard objects...  :/
> 
> Yes but I'd think most of them are (or am I wrong here?).

Not sure!  I know of some which aren't, so they do exist...

> Can't we have an
> opt-out annotation for the ones that aren't, similar to MOZ_IMPLICIT? i.e.
> MOZ_NONGUARD_STACK_CLASS

Yes, that is possible.  File a bug please!

> The guard object helpers are really ugly, FWIW. I wish we could just do that
> statically.

I agree.  But they also help people catch errors locally.

    
    

    
  
FWIW I'm on 42.0a2 (2015-08-26) and am seeing this... or at least something similar (:khuey pointed my issue at this bug but doesn't fully match summary)

    
    

    
  
Ehsan, as Bobby is in PTO, I am asking the question to you. do you think we should uplift that to 42? Thanks
Flags: needinfo?(ehsan)

    
    

    
  
(In reply to Sylvestre Ledru [:sylvestre] from comment #90)
> Ehsan, as Bobby is in PTO, I am asking the question to you. do you think we
> should uplift that to 42? Thanks

Yes, definitely.
Flags: needinfo?(ehsan)

    
    

    
  
Actually, we are going to disable e10s in aurora in the next few days, not taking it.

          status-firefox42:
          ? → affected

          tracking-firefox42:
          + → -
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: