Closed Bug 1743439 Opened 4 years ago Closed 4 years ago

Mark EventDispatcher::DispatchDOMEvent as MOZ_CAN_RUN_SCRIPT

Categories

(Core :: DOM: Events, task)

Product:
Core
Component:
DOM: Events
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
97 Branch
Tracking Flags:
Tracking Status
firefox97 --- fixed

People

(Reporter: saschanaz, Assigned: saschanaz)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Bug 1743439 - Part 1: Mark callers in dom/animation and dom/base as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
48 bytes, text/x-phabricator-request
Details | Review
Bug 1743439 - Part 2: Mark callers in dom/indexedDB and dom/workers as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
48 bytes, text/x-phabricator-request
Details | Review
Bug 1743439 - Part 3: Mark callers in dom/media as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
48 bytes, text/x-phabricator-request
Details | Review
Bug 1743439 - Part 4: Mark callers in layout as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
48 bytes, text/x-phabricator-request
Details | Review
Bug 1743439 - Part 5: Mark callers in dom/events and dom/html as MOZ_CAN_RUN_SCRIPT_BOUNDARY r=masayuki
48 bytes, text/x-phabricator-request
Details | Review
Bug 1743439 - Part 6: Mark EventDispatcher::DispatchDOMEvent as MOZ_CAN_RUN_SCRIPT r=masayuki
48 bytes, text/x-phabricator-request
Details | Review
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.

Attachment

General

Creator:
Created:
Updated:
Size: