Ting-Yu Chou [:ting] (away) Assignee
	Description • 9 years ago

See profile http://goo.gl/SiYCNo which was captured while launching Messages app, note there's another same IPC call and blocks main thread for ~120ms [137519,137640].

Gecko: 7649ffe28b67

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 1 • 9 years ago

TYLin, this relates to nsHtmlEditor::CanPaste(), any ideas how this can be improved? Thanks.

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 2 • 9 years ago

Currently, I don't have any ideas. I'll loop more developers who have been working on the copy and paste feature.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 3 • 9 years ago

Since ChildCommandDispatcher::Run() will SendEnableDisableCommands() to parent, can we check nsHtmlEditor::CanPaste() in parent instead of getting it from parent and then send it back?

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 4 • 9 years ago

The sync IPC is delayed since parent process main thread is busy doing refresh driver tick.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 5 • 9 years ago

The ChildCommandDispatcher is instantiated with this stack:

#0  nsGlobalWindow::UpdateCommands (this=0xb37bde70, anAction=..., 
    aSel=0xb14f7880, aReason=0)
    at ../../../gecko/dom/base/nsGlobalWindow.cpp:9675
#1  0xb56f7492 in nsDocViewerSelectionListener::NotifySelectionChanged (
    this=0xb0498c20, aReason=<optimized out>)
    at /home/ting/w/fx/os/aries-kk/gecko/layout/base/nsDocumentViewer.cpp:3580
#2  0xb57474ee in mozilla::dom::Selection::NotifySelectionListeners (
    this=0xb14f7880)
    at /home/ting/w/fx/os/aries-kk/gecko/layout/generic/nsSelection.cpp:5903
#3  0xb5747532 in nsFrameSelection::NotifySelectionListeners (this=0xb14f7880, 
    aType=<optimized out>)
    at /home/ting/w/fx/os/aries-kk/gecko/layout/generic/nsSelection.cpp:2240
#4  0xb574e7b6 in mozilla::dom::Selection::Collapse (this=0xb14f7880, 
    aParentNode=..., aOffset=55, aRv=...)
    at /home/ting/w/fx/os/aries-kk/gecko/layout/generic/nsSelection.cpp:4900
#5  0xb574e994 in Collapse (aOffset=<optimized out>, 
    aParentNode=<optimized out>, this=<optimized out>)
    at /home/ting/w/fx/os/aries-kk/gecko/layout/generic/nsSelection.cpp:4838
#6  mozilla::dom::Selection::Collapse (this=<optimized out>, 
    aParentNode=<optimized out>, aOffset=<optimized out>)
    at /home/ting/w/fx/os/aries-kk/gecko/layout/generic/nsSelection.cpp:4832
#7  0xb561eb8a in nsEditor::EndOfDocument (this=0xb6a80f90)
    at /home/ting/w/fx/os/aries-kk/gecko/editor/libeditor/nsEditor.cpp:1116
#8  0xb5648944 in Init (aEditor=0x0, this=0xafea0c00)
    at /home/ting/w/fx/os/aries-kk/gecko/editor/libeditor/nsTextEditRules.cpp:133
#9  nsTextEditRules::Init (this=this@entry=0xafea0c00, 
    aEditor=aEditor@entry=0xb6a80f90)                                   [0/1815]
    at /home/ting/w/fx/os/aries-kk/gecko/editor/libeditor/nsTextEditRules.cpp:112
#10 0xb5634a4a in nsHTMLEditRules::Init (this=0xafea0c00, aEditor=0xb6a80f90)
    at /home/ting/w/fx/os/aries-kk/gecko/editor/libeditor/nsHTMLEditRules.cpp:246
#11 0xb562a43a in nsHTMLEditor::InitRules (this=0xb6a80f90)
    at /home/ting/w/fx/os/aries-kk/gecko/editor/libeditor/nsHTMLEditor.cpp:501
#12 0xb5641a48 in nsPlaintextEditor::EndEditorInit (this=0xb6a80f90)
    at /home/ting/w/fx/os/aries-kk/gecko/editor/libeditor/nsPlaintextEditor.cpp:204
#13 0xb564236c in nsAutoEditInitRulesTrigger::~nsAutoEditInitRulesTrigger (
    this=0xbe9bf83c, __in_chrg=<optimized out>)
    at /home/ting/w/fx/os/aries-kk/gecko/editor/libeditor/nsTextEditUtils.cpp:103
#14 0xb5637786 in nsHTMLEditor::Init (this=0xb6a80f90, aDoc=<optimized out>, 
    aRoot=<optimized out>, aSelCon=<optimized out>, aFlags=1024, 
    aInitialValue=...)
    at /home/ting/w/fx/os/aries-kk/gecko/editor/libeditor/nsHTMLEditor.cpp:298
#15 0xb56558c2 in nsEditingSession::SetupEditorOnWindow (
    this=this@entry=0xaf94b100, aWindow=aWindow@entry=0xb37bde80)
    at /home/ting/w/fx/os/aries-kk/gecko/editor/composer/nsEditingSession.cpp:455
#16 0xb5655de6 in nsEditingSession::MakeWindowEditable (this=0xaf94b100, 
    aWindow=0xb37bde80, aEditorType=0xb6140ec6 "html", 
    aDoAfterUriLoad=<optimized out>, aMakeWholeDocumentEditable=false, 
    aInteractive=true)

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 6 • 9 years ago

Shouldn't we set mSelectionWasCollapsed [1] default true if initialize always collapse the selection?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#170

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 7 • 9 years ago

Traced the code but still don't understand why child process asks parent whether a paste command is enabled or not and then sends the result back to parent.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #6)
> Shouldn't we set mSelectionWasCollapsed [1] default true if initialize
> always collapse the selection?
Probably yes indeed. It has been false for ages, at least from 2000-02-15

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsDocumentViewer.cpp&rev=1.587&mark=247#245

	Neil Deakin
	Comment 9 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #7)
> Traced the code but still don't understand why child process asks parent
> whether a paste command is enabled or not and then sends the result back to
> parent.

When the selection or focus changes, the child process gathers up a list of commands that are enabled and those that are disabled, and informs the parent process so that it can update the UI accordingly. For the paste command, whether the command is enabled depends on two pieces of information: what data is on the clipboard currently while is available in the parent process only, and what is currently selected/focused which is available in the child process only.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 10 • 9 years ago

(In reply to Neil Deakin from comment #9)
> When the selection or focus changes, the child process gathers up a list of
> commands that are enabled and those that are disabled, and informs the
> parent process so that it can update the UI accordingly. For the paste
> command, whether the command is enabled depends on two pieces of
> information: what data is on the clipboard currently while is available in
> the parent process only, and what is currently selected/focused which is
> available in the child process only.

Thanks for explaining me.

What if child process gathers up a list of enabled/disabled commands itself knows, pass to parent, and parent to gather remaining enabled/disabled commands parent knows before updating UI.

This eliminate IPCs and the chance parent to block child.

	Neil Deakin
	Comment 11 • 9 years ago

Determining whether the paste command is enabled requires knowing what is focused and/or 
selected, which only the child can determine.

Is the performance issue caused only by the update caused by selection changes? That is, does disabling the call to UpdateCommands in nsDocViewerSelectionListener::NotifySelectionChanged reduce or eliminate the issue?

If so, the simplest solution might be to just not update the paste command in response to a selection change. Possibly only not doing this during startup, in case there's some case I'm not thinking of where it matters.

A more complex and less extensible solution might be to add some special commands that indicate to the parent which clipboard checks to perform. That means hard-coding some things though.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 12 • 9 years ago

(In reply to Neil Deakin from comment #11)
> Is the performance issue caused only by the update caused by selection
> changes? That is, does disabling the call to UpdateCommands in
> nsDocViewerSelectionListener::NotifySelectionChanged reduce or eliminate the
> issue?

Yes. And more specifically, UpdateCommands() is triggered by the collapse of initializing nsHTMLEditor.

> If so, the simplest solution might be to just not update the paste command
> in response to a selection change. Possibly only not doing this during
> startup, in case there's some case I'm not thinking of where it matters.

It seems the simplest solution is to skip the initialized collapse, see comment 6.

	Johnny Stenback (:jst)
	Comment 14 • 9 years ago

Comment on attachment 8653880 [details] [diff] [review]
patch v1

r=jst as this seems very reasonable to me.

Also, as a followup it might be possible to change the IPC that goes child->parent to be async if the sole purpose of the child telling the parent about selection state changes is so that the parent can update the UI etc.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 15 • 9 years ago

Thanks for prompt review. Updated reviewer.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e7dda88974

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 16 • 9 years ago

(In reply to Ting-Yu Chou [:ting] from comment #15)
> Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e7dda88974

Forgot to include unit tests, retry: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82bb155f8c78

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 17 • 9 years ago

Try looks good.

	Pulsebot
	Comment 18 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d74734792df

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/2d74734792df

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 20 • 9 years ago

Curious, did you test that cut/copy/paste state in the parent process is update properly when
one switches tab?
(I couldn't see any issues there.)

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 21 • 9 years ago

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #20)
> Curious, did you test that cut/copy/paste state in the parent process is
> update properly when
> one switches tab?
> (I couldn't see any issues there.)

No, could you please tell me where is the state updated in parent? I am not sure how to test this.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 22 • 9 years ago

Well, I was actuallly just looking at the state of the paste/copy/cut in the edit menu.
Not sure if something else should be checked too.

	Ting-Yu Chou [:ting] (away) Assignee
	Comment 23 • 9 years ago

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #22)
> Well, I was actuallly just looking at the state of the paste/copy/cut in the
> edit menu.
> Not sure if something else should be checked too.

I just tried that with some different scenarios, and I didn't see anything wrong. I also don't know what else should be checked.

TYLin, are there test cases cover what :smaug is considering in comment 20?

	Ting-Yu Lin [:TYLin] (UTC-8)
	Comment 24 • 9 years ago

I do not know whether there's any test covered the case in comment 20. This seems somewhat related to editor. Ehsan, any ideas?

	(no longer active)
	Comment 25 • 9 years ago

I'm pretty sure Michael added some tests for that very recently, so redirecting the needinfo to him.

That being said, I don't think those tests check what happens during tab switching.  We should probably extend them for tab switching.

	Nika Layzell [:nika] (ni? for response)
	Comment 26 • 9 years ago

I think the tests Ehsan is talking about are these: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/browser_bug1170531.js