Sync IPC ClipboardHasType could block >90ms and hurts app startup

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ting, Assigned: ting)

Tracking

Trunk
mozilla43
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.5+, firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 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
(Assignee)

Updated

3 years ago
OS: Unspecified → Gonk (Firefox OS)
Hardware: Unspecified → ARM
(Assignee)

Comment 1

3 years ago
TYLin, this relates to nsHtmlEditor::CanPaste(), any ideas how this can be improved? Thanks.
Flags: needinfo?(tlin)
Currently, I don't have any ideas. I'll loop more developers who have been working on the copy and paste feature.
(Assignee)

Comment 3

3 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?
(Assignee)

Comment 4

3 years ago
The sync IPC is delayed since parent process main thread is busy doing refresh driver tick.
Depends on: 1110625
(Assignee)

Comment 5

3 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)
(Assignee)

Comment 6

3 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
(Assignee)

Comment 7

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

Comment 8

3 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

Comment 9

3 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.
(Assignee)

Comment 10

3 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.
Flags: needinfo?(enndeakin)

Comment 11

3 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.
Flags: needinfo?(enndeakin)
(Assignee)

Comment 12

3 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.
(Assignee)

Comment 13

3 years ago
Created attachment 8653880 [details] [diff] [review]
patch v1
Flags: needinfo?(tlin)
Attachment #8653880 - Flags: review?(jst)
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.
Attachment #8653880 - Flags: review?(jst) → review+
(Assignee)

Comment 15

3 years ago
Created attachment 8653926 [details] [diff] [review]
patch v2

Thanks for prompt review. Updated reviewer.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95e7dda88974
Attachment #8653880 - Attachment is obsolete: true

Updated

3 years ago
Blocks: 1180696
feature-b2g: --- → 2.5+

Updated

3 years ago
Assignee: nobody → janus926
Status: NEW → ASSIGNED
(Assignee)

Comment 17

3 years ago
Try looks good.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2d74734792df
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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.)
(Assignee)

Comment 21

3 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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 23

3 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?
Flags: needinfo?(tlin)
I do not know whether there's any test covered the case in comment 20. This seems somewhat related to editor. Ehsan, any ideas?
Flags: needinfo?(tlin) → needinfo?(ehsan)

Comment 25

3 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.
Flags: needinfo?(ehsan) → needinfo?(michael)

Comment 26

3 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
Flags: needinfo?(michael)
You need to log in before you can comment on or make changes to this bug.