Closed Bug 1438397 Opened 6 years ago Closed 6 years ago

RepaintSelectionRunner might run after the presshell is destroyed

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(1 file)

In bug 1432396, I did add an assertion that checks the docshell is NOT being destroyed in nsDocShell::EnsureTransferableHookData() [1].  With the assertion, testing/marionette/harness/marionette_harness/tests/unit/test_elementsize_chrome.py TestElementSizeChrome.testShouldReturnTheSizeOfAnInput hits the assertion on MacOSX [2].

We should bail out from RepaintSelectionRunner if the docshell (I think the presshell too) is being destroyed.

[1] https://hg.mozilla.org/try/rev/12f8796def09f46b6205309f4183238862279e7a#l1.351
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=162291707&repo=try&lineNumber=5731

https://treeherder.mozilla.org/#/jobs?repo=try&revision=099673b8ac9cc8199c3c969505101036c3638d3a
Summary: RepaintSelectionRunner might runs after the presshell is destroyed → RepaintSelectionRunner might run after the presshell is destroyed
Blocks: 1432396
Comment on attachment 8951126 [details]
Bug 1438397 - Don't process RepaintSelection() if the presshell is being destroyed.

https://reviewboard.mozilla.org/r/220384/#review226344

::: editor/libeditor/EditorBase.cpp:5235
(Diff revision 1)
>    {
>    }
>  
>    NS_IMETHOD Run() override
>    {
> +    nsCOMPtr<nsIPresShell> shell = do_QueryInterface(mSelectionController);

Hmm, we need QI here :-(
Attachment #8951126 - Flags: review?(masayuki) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a281dd360b0a
Don't process RepaintSelection() if the presshell is being destroyed. r=masayuki
https://hg.mozilla.org/mozilla-central/rev/a281dd360b0a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Would it be cleaner to just check mIsDestroying in PresShell instead of doing QI?

Happy to submit a patch if so.
Flags: needinfo?(masayuki)
I guess it's not a big deal and the other callers may not need the check... shrug.

Sorry for the drive-by comment, I just saw the commit in the pushlog :)
Flags: needinfo?(masayuki)
Yeah, in most cases, the QI is necessary anyway. We need to improve the such major case.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: