1543315 - Mark methods of nsIPresShell and mozilla::PresShell as MOZ_CAN_RUN_SCRIPT (or at least MOZ_CAN_RUN_SCRIPT_BOUNDARY) if necessary

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'll create some patches for example. Then, you're welcome to post patches instead of me. At that time, please let me know which method will you mark.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 1: Mark EventDispatchingCallback as MOZ_CAN_RUN_SCRIPT

First of all, we should mark nsIPresShell::FlushPendingNotifications() as
MOZ_CAN_RUN_SCRIPT as soon as possible. Therefore, I'll mark all its callers
in PresShell as MOZ_CAN_RUN_SCRIPT first.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 2: Mark VerifyIncrementalReflow() and DoVerifyReflow() as MOZ_CAN_RUN_SCRIPT_BOUNDARY

They are debug build only methods. So, callers of them shouldn't be marked
as MOZ_CAN_RUN_SCRIPT only for them. Therefore, MOZ_CAN_RUN_SCRIPT_BOUNDARY
must be better.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 3: Mark WillPaint() as MOZ_CAN_RUN_SCRIPT

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 4: Mark ResizeReflow() as MOZ_CAN_RUN_SCRIPT

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 5: Mark ResizeReflowIgnoreOverride() as MOZ_CAN_RUN_SCRIPT

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 6: Mark ProcessReflowCommands() as MOZ_CAN_RUN_SCRIPT

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 7: Mark DidDoReflow() as MOZ_CAN_RUN_SCRIPT

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 8: Mark HandlePostedReflowCallbacks() as MOZ_CAN_RUN_SCRIPT

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 9: Mark nsIPresShell::FlushPendingNotifications() as MOZ_CAN_RUN_SCRIPT

So, this patch makes all caller of it safe including its arguments unless
they come from other methods.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfce19e74038
part 1: Mark EventDispatchingCallback as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/f1a796212f61
part 2: Mark VerifyIncrementalReflow() and DoVerifyReflow() as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=smaug
https://hg.mozilla.org/integration/autoland/rev/33a2dec4cfd3
part 3: Mark WillPaint() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/10608c915cc5
part 4: Mark ResizeReflow() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/4f65a6a58238
part 5: Mark ResizeReflowIgnoreOverride() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/7bc3d65d627f
part 6: Mark ProcessReflowCommands() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/32c1716986d2
part 7: Mark DidDoReflow() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/a25a14f7150d
part 8: Mark HandlePostedReflowCallbacks() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/a57a60c0278d
part 9: Mark nsIPresShell::FlushPendingNotifications() as MOZ_CAN_RUN_SCRIPT r=smaug

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/dfce19e74038
https://hg.mozilla.org/mozilla-central/rev/f1a796212f61
https://hg.mozilla.org/mozilla-central/rev/33a2dec4cfd3
https://hg.mozilla.org/mozilla-central/rev/10608c915cc5
https://hg.mozilla.org/mozilla-central/rev/4f65a6a58238
https://hg.mozilla.org/mozilla-central/rev/7bc3d65d627f
https://hg.mozilla.org/mozilla-central/rev/32c1716986d2
https://hg.mozilla.org/mozilla-central/rev/a25a14f7150d
https://hg.mozilla.org/mozilla-central/rev/a57a60c0278d

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 10: Mark nsIPresShell::GoToAnchor() and nsIPresShell::ScrollToAnchor() as MOZ_CAN_RUN_SCRIPT

Next, we should mark PresShell::ScrollContentIntoView() as
MOZ_CAN_RUN_SCRIPT because it's used widely.

This patch marks its PresShell users, GoToAnchor() and ScrollToAnchor(),
as MOZ_CAN_RUN_SCRIPT. Additionally, this patch moves them from
nsIPresShell to PresShell because all callers refers PresShell directly.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 11: Mark nsIPresShell::ScrollContentIntoView() as MOZ_CAN_RUN_SCRIPT

This patch marks ScrollContentIntoView() as MOZ_CAN_RUN_SCRIPT and changing
some callers of them to guarantee thar their parent callers are also safe.

Additionally, this patch moves it from nsIPresShell to PresShell because
all callers can refer PresShell directly.

Unfortunately, this patch changes a lot of methods in autocomplete and satchel
since this patch needs to mark some interface methods as can_run_script and
they are called each other. This means that autocomplete module is really
sensitive like editor module. Perhaps, autocomplete and satchel should do
scroll asynchronously and unmark some of them as MOZ_CAN_RUN_SCRIPT again.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 12: Mark nsIPresShell::DoScrollContentIntoView() as MOZ_CAN_RUN_SCRIPT

Now, we can mark DoScrollContentIntoView() as MOZ_CAN_RUN_SCRIPT and move
it from nsIPresShell to PresShell with a member.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d921a5274f5
part 10: Mark nsIPresShell::GoToAnchor() and nsIPresShell::ScrollToAnchor() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/7d67598c9043
part 11: Mark nsIPresShell::ScrollContentIntoView() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/7b69b606bb29
part 12: Mark nsIPresShell::DoScrollContentIntoView() as MOZ_CAN_RUN_SCRIPT r=smaug

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/3d921a5274f5
https://hg.mozilla.org/mozilla-central/rev/7d67598c9043
https://hg.mozilla.org/mozilla-central/rev/7b69b606bb29

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 13: Mark PresShell::Paint() as MOZ_CAN_RUN_SCRIPT

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 14: Mark PresShell::WillPaintWindow() and PresShell::DidPaintWindow() as MOZ_CAN_RUN_SCRIPT

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 15: Mark PresShell::ScrollFrameRectIntoView() as MOZ_CAN_RUN_SCRIPT

Unfortunately, EventChainVisitor does not grab the nsPresContext with
RefPtr by itself. Therefore, there is no guarantee of the lifetime without
checking the origin when its subclasses are instantiated. This patch changes
it and subclasses to MOZ_STACK_CLASS since only EventDispatcher::Dispatch()
creates them in the stack with given nsPresContext. Additionally, it's
already been marked as MOZ_CAN_RUN_SCRIPT_BOUNDARY. Therefore, thensPresContextinstance has already been guaranteed its lifetime by the caller. For making this fact stronger, this patch marks their constructors asMOZ_CAN_RUN_SCRIPT. Therefore, nobody can create those instances without guaranteeing the lifetime ofnsPresContextanddom::Event. Note that it may look like thatmPresContextofEventChainPostVisitoris not guaranteed. However,EventChainPreVisitorwhich givesnsPresContextto it is also a stack only class. So, it won't be deleted beforeEventChainPostVisitor` instance.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 16: Mark PresShell::HandleEventWithTarget() as MOZ_CAN_RUN_SCRIPT

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59633bc3f761
part 13: Mark PresShell::Paint() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/e2969c920810
part 14: Mark PresShell::WillPaintWindow() and PresShell::DidPaintWindow() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/b4cbda10ffaf
part 15: Mark PresShell::ScrollFrameRectIntoView() as MOZ_CAN_RUN_SCRIPT r=smaug
https://hg.mozilla.org/integration/autoland/rev/801ff1a58ffc
part 16: Mark PresShell::HandleEventWithTarget() as MOZ_CAN_RUN_SCRIPT r=smaug

Comment 23

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/59633bc3f761
https://hg.mozilla.org/mozilla-central/rev/e2969c920810
https://hg.mozilla.org/mozilla-central/rev/b4cbda10ffaf
https://hg.mozilla.org/mozilla-central/rev/801ff1a58ffc

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 17: Mark PresShell::HandleDOMEventWithTarget() as MOZ_CAN_RUN_SCRIPT

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e754d9ad197d
part 17: Mark PresShell::HandleDOMEventWithTarget() as MOZ_CAN_RUN_SCRIPT r=smaug

Comment 26

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e754d9ad197d

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

There are some methods which must be marked as MOZ_CAN_RUN_SCRIPT before closing this bug. I don't have much time for now, but I'll come back here.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

smaug:

I have a question, I try to mark PresShell::MaybeReleaseCapturingContent() because it calls nsFrameSelection::SetDragState(false) and if it actually changes the state to false (i.e., drag state was true), it notifies selection listeners. However, it causes PresShell::Freeze() and PresShell::Destroy() also unsafe methods and that causes too many callers being also MOZ_CAN_RUN_SCRIPT even though it's rare case.

I wonder, it must be reasonable to change SetDragState(false) in this case as using NS_DispatchToCurrentThread() if it's okay. Although, I don't know whether notifying selection listeners at that time is actually required or not. WDYT?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

• https://hg.mozilla.org/try/rev/417a9a73b356dab0ce1f288da3a92640bf492588 (If we mark Freeze() as MOZ_CAN_RUN_SCRIPT)
• https://hg.mozilla.org/try/rev/853b4b06b7c6e70dcfa30bedc9ee81b49f3e467a (If we mark Destroy() as MOZ_CAN_RUN_SCRIPT)

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

That sounds scary. We'd be in drag state and although we really aren't, and we could process some more user input events before handling
SetDragState(false).
Could we make only some parts of the code inside SetDragState() asynchronous?

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 18: Mark `PresShell::FireResizeEvent()` as `MOZ_CAN_RUN_SCRIPT` r=smaug!

It dispatches a DOM event so that it should be marked as MOZ_CAN_RUN_SCRIPT.

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 19: Mark `PresShell::ReconstructFrames()` as `MOZ_CAN_RUN_SCRIPT` r=smaug!

It calls Document::FlushPendingNotification() so that we should mark it
as MOZ_CAN_RUN_SCRIPT.

And the method calls it of mDocument and mDocument is never modified
after it's initialized. Therefore, we can move the initializer to the
constructor and make RefPtr<Document> to RefPtr<Document> const. Thus,
we can avoid unnecessary auto RefPtr.

Depends on D55801

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 20: Mark `PresShell::ContentStateChanged()` as `MOZ_CAN_RUN_SCRIPT_BOUNDARY` r=smaug!

While it calls RestyleManager::ContentStateChanged(), it blocks script
with nsAutoCauseReflowNotifier. Therefore, it should be marked as
MOZ_CAN_RUN_SCRIPT_BOUNDARY at least (looks like the other override,
DocAccessible::ContentStateChanged() does not run script).

I'm not sure about the lifetime of RestyleManager. It's destroyed when
nsPresContext::DetachPresShell() is called. It's called by
PresShell::Destroy() and destructor of nsPresContext. The latter is
safe since PresShell owns mPresContext. However, I'm not sure about
the former. It might be better to create blocker of synchronous handling of
PresShell::Destroy().

And also this does not make Document::ContentStateChanged() use
RefPtr<PresShell> at calling it because it might cause performance
regression, but it does not do anything after destroying
nsAutoCauseReflowNotifier.

Depends on D55803

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1543315 - part 21: Mark `PresShell::DidCauseReflow()` as `MOZ_CAN_RUN_SCRIPT` r=smaug!

It removes a script blocker. Therefore, although it depends on the caller
whether it causes running script or not. However, we should mark it as
MOZ_CAN_RUN_SCRIPT for safer code.

It's called only by the destructor of nsAutoCauseReflowNotifier. Therefore,
this patch also marks its constructor as MOZ_CAN_RUN_SCRIPT for making
each creator method marked as MOZ_CAN_RUN_SCRIPT or
MOZ_CAN_RUN_SCRIPT_BOUNDARY.

Most of the creators is mutation listener methods. However, PresShell
does nothing after destroying nsAutoCauseReflowNotifier. Therefore,
this patch does not change the callers in MutationObserver.cpp to use
RefPtr<PresShell> at calling them because changing it may cause performance
regression.

Perhaps, we should create another methods of WillCauseReflow() and
DidCauseReflow() to avoid unnecessary MOZ_CAN_RUN_SCRIPT marking.
However, I'm not sure whether most callers may run script or not because
of outside of my knowledge.

Depends on D55804

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ded9882e6cc
part 18: Mark `PresShell::FireResizeEvent()` as `MOZ_CAN_RUN_SCRIPT` r=smaug

Comment 37

[Alexandru Michis [:malexandru]]

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

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26ea42782e3e
part 19: Mark `PresShell::ReconstructFrames()` as `MOZ_CAN_RUN_SCRIPT` r=smaug

Comment 39

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/26ea42782e3e

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7626f9316348
part 20: Mark `PresShell::ContentStateChanged()` as `MOZ_CAN_RUN_SCRIPT_BOUNDARY` r=smaug

Comment 41

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/7626f9316348

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a23114f53100
part 21: Mark `PresShell::DidCauseReflow()` as `MOZ_CAN_RUN_SCRIPT` r=smaug

Comment 43

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/a23114f53100

Comment 44

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?

Comment 45

[Sean Voisen (:svoisen)]

Hi Masayuki, Do you still want this left open?

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, still there are some methods which should be marked as MOZ_CAN_RUN_SCRIPT. But I don't have much time to work on this at least next several months. So, I'd like to keep this open, but it make bug management harder, cloning this bug without assignee is also fine to me.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Okay, I'll mark this as fixed and when I'm back this issue, I'll file new bugs which blocks this bug.