EditorBase should cache pointer to mSelConWeak, mPlaceHolderTxn and mDocWeak

RESOLVED FIXED in Firefox 56

Status

()

Core
Editor
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 1 bug)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Currently, EditorBase::mSelConWeak, EditorBase::mPlaceHolderTxn and EditorBase::mDocWeak are nsWeakPtr. However, they require QI before use but that appears in some profiles. So, I think that we should cache it and use nsWeakPtr only for checking if the pointer is still available.
Ah, it might be able to use mfbt/WeakPtr.h here.
https://searchfox.org/mozilla-central/source/mfbt/WeakPtr.h
Oh, but EditorBase doesn't treat document as nsDocument. It treats document as nsIDocument or nsIDOMDocument. So, WeakPtr<nsDocument> isn't available in EditorBase. Sigh.
The fix improves the performance of bug 1346723 about 3%.
Oh, but this improves the score of the testcase for bug 1370754 (after setting innerHTML) about 10%.
(Assignee)

Updated

10 months ago
Blocks: 1370754
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #10)
> Oh, but this improves the score of the testcase for bug 1370754 (after
> setting innerHTML) about 10%.

Oops, not 10%, but 1%.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
bsmedberg:

Could you review the XPCOM part of the patch?

Updated

10 months ago
Attachment #8878455 - Flags: review?(benjamin) → review?(nfroyd)

Comment 16

10 months ago
mozreview-review
Comment on attachment 8878454 [details]
Bug 1372829 - part1: Make mozilla::PlaceholderTransaction inherit mozilla::SupportsWeakPtr instead of nsSupportsWeakReference

https://reviewboard.mozilla.org/r/149788/#review154794
Attachment #8878454 - Flags: review?(m_kato) → review+

Comment 17

10 months ago
mozreview-review
Comment on attachment 8878455 [details]
Bug 1372829 - part2: mozilla::EditorBase should cache raw pointer of nsISelectionController and nsIDocument with nsWeakPtr

https://reviewboard.mozilla.org/r/149790/#review154796

::: editor/libeditor/EditorBase.h:237
(Diff revision 1)
>  
>  public:
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(EditorBase, nsIEditor)
>  
> +  bool IsInitialized() { return !!mDocumentWeak; }

bool IsInitialized() const?
Attachment #8878455 - Flags: review?(m_kato) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

10 months ago
mozreview-review-reply
Comment on attachment 8878455 [details]
Bug 1372829 - part2: mozilla::EditorBase should cache raw pointer of nsISelectionController and nsIDocument with nsWeakPtr

https://reviewboard.mozilla.org/r/149790/#review154796

> bool IsInitialized() const?

Sure!

Comment 21

10 months ago
mozreview-review
Comment on attachment 8878455 [details]
Bug 1372829 - part2: mozilla::EditorBase should cache raw pointer of nsISelectionController and nsIDocument with nsWeakPtr

https://reviewboard.mozilla.org/r/149790/#review155226
Attachment #8878455 - Flags: review?(nfroyd) → review+

Comment 22

10 months ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3d35d8ad784b
part1: Make mozilla::PlaceholderTransaction inherit mozilla::SupportsWeakPtr instead of nsSupportsWeakReference r=m_kato
https://hg.mozilla.org/integration/autoland/rev/971e73db7835
part2: mozilla::EditorBase should cache raw pointer of nsISelectionController and nsIDocument with nsWeakPtr r=froydnj,m_kato

Comment 23

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3d35d8ad784b
https://hg.mozilla.org/mozilla-central/rev/971e73db7835
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I'm now getting crashes like:
#0  0x00007fffe9adb528 in mozilla::CachedWeakPtr<nsIDocument, nsISupports>::get() const (this=<optimized out>) at /home/smaug/mozilla/hg/vsync/ff_build_opt/dist/include/mozilla/EditorBase.h:198
#1  0x00007fffe9adb528 in mozilla::EditorBase::GetDocument() (this=0x7ffff6c00008) at /home/smaug/mozilla/hg/vsync/editor/libeditor/EditorBase.cpp:566
#2  0x00007fffe9adb528 in mozilla::EditorBase::GetPresShell() (this=<optimized out>) at /home/smaug/mozilla/hg/vsync/editor/libeditor/EditorBase.cpp:587
#3  0x00007fffe9adb528 in mozilla::EditorBase::GetSelectionController() (this=0x7ffff6c00008) at /home/smaug/mozilla/hg/vsync/editor/libeditor/EditorBase.cpp:642
#4  0x00007fffe9ad5cc5 in mozilla::EditorBase::GetSelection(mozilla::SelectionType, nsISelection**) (aSelectionType=<optimized out>, this=<optimized out>, aSelection=<optimized out>)
    at /home/smaug/mozilla/hg/vsync/editor/libeditor/EditorBase.cpp:668
#5  0x00007fffe9ad5cc5 in mozilla::EditorBase::GetSelection(mozilla::SelectionType) (this=0x7ffff6c00008, aSelectionType=mozilla::SelectionType::eNormal)
    at /home/smaug/mozilla/hg/vsync/editor/libeditor/EditorBase.cpp:679
#6  0x00007fffe9af03ce in mozilla::HTMLEditor::GetSelectionContainer() (this=0x7ffff6c00008) at /home/smaug/mozilla/hg/vsync/editor/libeditor/HTMLEditor.cpp:4849


when trying to call GetSelectionContainer
(In reply to Olli Pettay [:smaug] from comment #24)
> I'm now getting crashes like:
> #0  0x00007fffe9adb528 in mozilla::CachedWeakPtr<nsIDocument,
> nsISupports>::get() const (this=<optimized out>) at
> /home/smaug/mozilla/hg/vsync/ff_build_opt/dist/include/mozilla/EditorBase.h:
> 198

Let me know the crash reason like nullptr access or heap-use-after-free?
Flags: needinfo?(bugs)
Maybe it was gdb showing wrong data after all while I was writing a patch which still contained some other issues. I'll re-report if I see the issue again.
Flags: needinfo?(bugs)
Okay. FYI: we have no report crashed in CachedWeakPtr yet.

Updated

9 months ago
Depends on: 1385384
You need to log in before you can comment on or make changes to this bug.