Closed Bug 1743439 Opened 10 months ago Closed 10 months ago

Mark EventDispatcher::DispatchDOMEvent as MOZ_CAN_RUN_SCRIPT

Categories

(Core :: DOM: Events, task)

task

Tracking

()

RESOLVED FIXED
97 Branch
Tracking Status
firefox97 --- fixed

People

(Reporter: saschanaz, Assigned: saschanaz)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Since it also clearly can.

See Also: → 1743443
Assignee: nobody → krosylight
Status: NEW → ASSIGNED

(In reply to Peter Van der Beken [:peterv] from comment #7)

Can we please make sure to follow https://searchfox.org/mozilla-central/rev/4ca2f73ae9346709d39de2c8fe33874e4203b9e6/mfbt/Attributes.h#499-501

Hmm, about the former, it's the best for all readers of our code, but it's hard to add comment that which causes the MOZ_CAN_RUN_SCRIPT or its variant because if you're now marking methods as so, you obviously know which call(s) requires the attribute. On the other hand, if you touch methods which are marked as MOZ_CAN_RUN_SCRIPT, it's hard to modify the comment (especially for that reviewers won't realize that it's already been marked, but adding new call). Additionally, there are only a couple of scenarios why MOZ_CAN_RUN_SCRIPT_BOUNDARY is used instead of MOZ_CAN_RUN_SCRIPT. E.g., it's a override of a virtual method which is used globally, or there are too much callers to mark its callers in the bug. So I think that there should be MOZ_CAN_RUN_SCRIPT_BOUNDARY_DUE_TO_* if the reason is important.

Pushed by krosylight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a919ef50cf33
Part 1: Mark callers in dom/animation and dom/base as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/7ad2746ec876
Part 2: Mark callers in dom/indexedDB and dom/workers as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/ba0fa3a8fa0e
Part 3: Mark callers in dom/media as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/23202094bc00
Part 4: Mark callers in layout as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/0376e73c7dce
Part 5: Mark callers in dom/events and dom/html as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/792c405d4afc
Part 6: Mark EventDispatcher::DispatchDOMEvent as MOZ_CAN_RUN_SCRIPT r=masayuki

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #8)

Hmm, about the former, it's the best for all readers of our code, but it's hard to add comment that which causes the MOZ_CAN_RUN_SCRIPT or its variant because if you're now marking methods as so, you obviously know which call(s) requires the attribute.

I think the main point is that MOZ_CAN_RUN_SCRIPT_BOUNDARY opts us out of the checking that MOZ_CAN_RUN_SCRIPT gives, and so it should be the exception rather than the norm. Boris specifically added documentation to make sure that we don't go down the path of opting out without a plan to remove the opt-out, because that defeats the purpose. Hence the demand for a plan for removal of any new usage of MOZ_CAN_RUN_SCRIPT_BOUNDARY (see https://searchfox.org/mozilla-central/rev/4ca2f73ae9346709d39de2c8fe33874e4203b9e6/mfbt/Attributes.h#486-498), and the comment to document that plan so we know what to do in the future. Note that this is about MOZ_CAN_RUN_SCRIPT_BOUNDARY only, not about usage of MOZ_CAN_RUN_SCRIPT.

Backout by malexandru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7fd0d3cdbe10
Backed out 6 changesets for causing bustages in AnimationEventDispatcher.h

Ah, I see. Thank you for your explanation (I'm going to go to bed...)

Pushed by krosylight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a80f721def5b
Part 1: Mark callers in dom/animation and dom/base as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/08fb162e95bc
Part 2: Mark callers in dom/indexedDB and dom/workers as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/99f19fbc0f57
Part 3: Mark callers in dom/media as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/97eeec406ee0
Part 4: Mark callers in layout as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/4ecdd4f7a95f
Part 5: Mark callers in dom/events and dom/html as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
https://hg.mozilla.org/integration/autoland/rev/191c50bba814
Part 6: Mark EventDispatcher::DispatchDOMEvent as MOZ_CAN_RUN_SCRIPT r=masayuki
You need to log in before you can comment on or make changes to this bug.