Closed Bug 1538979 Opened 9 months ago Closed 7 months ago

Call `willDestroy` and `didDestroy` lifecycle methods on JSWindowActor

Categories

(Core :: DOM: Content Processes, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Fission Milestone M3
Tracking Status
firefox68 --- fixed

People

(Reporter: Nika, Assigned: jdai)

References

(Depends on 1 open bug)

Details

(Whiteboard: [4/16] 2 patches need revision)

Attachments

(3 files, 3 obsolete files)

The willDestroy method, if present, should be called at the last opportunity to send messages to the remote side, giving implementors the chance to clean up and send final messages. The didDestroy action is called after the actor is no longer able to receive any more messages. Messages may be received between willDestroy and didDestroy, but they may not be sent.

In terms of implementation, like many things with Window Actors, this depends on the side.

Parent Side

The willDestroy method should be called before calling Send__delete__ in RecvDestroy on the WindowGlobalParent actor: https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/dom/ipc/WindowGlobalParent.cpp#152

We don't want to call this method if our actor is already closed (e.g. due to TabParent being destroyed), as we won't be able to send messages anymore, so only call it inside of the mentioned conditional :-).

The didDestroy method should be called immediately after notifying observers of "window-global-destroyed" in ActorDestroy: https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/dom/ipc/WindowGlobalParent.cpp#242

Child Side

The willDestroy method should be called immediately before SendDestroy in WindowGlobalChild: https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/dom/ipc/WindowGlobalChild.cpp#109

We don't want to call this method if our actor is already closed (e.g. due to TabChild being destroyed), as we won't be able to send messages anymore, so only call it inside of the mentioned conditional :-).

The didDestroy method should be called at the end of ActorDestroy on WindowGlobalChild: https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/dom/ipc/WindowGlobalChild.cpp#190

:jdai, would you be able to work on this next?

Flags: needinfo?(jdai)
Blocks: 1538981

(In reply to :Nika Layzell from comment #1)

:jdai, would you be able to work on this next?

Sure, I'd like to. :)

Status: NEW → ASSIGNED
Flags: needinfo?(jdai)
  • Add WillDestroy and Destroy lifecycle methods on JSWindowActor.
  • After WillDestory allows to send final messages.
  • WindowGlobalParent/WindowGlobalChild::GetActor should fail after actor
    is closed.
  • Clear mManager to nullptr in ActorDestroy.
  • Add an optional cleanup callback function.
Whiteboard: [4/16] 2 patches need revision
Fission Milestone: M2 → M3
Attachment #9058579 - Attachment is obsolete: true
Attachment #9058580 - Attachment is obsolete: true
Attachment #9058581 - Attachment is obsolete: true
Pushed by jdai@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9df2b856b655
Part 1: Clear mManager in ActorDestroy and disallow sending message while Destroying; r=nika
https://hg.mozilla.org/integration/autoland/rev/8e065761738c
Part 2: Add WillDestroy and DidDestroy lifecycle methods on JSWindowActor; r=nika
https://hg.mozilla.org/integration/autoland/rev/a098226e4211
Part 3: Add testcase to test JSWindowActor's lifecycle; r=nika

Backed out for bustage on JSWindowActor.cpp

backout: https://hg.mozilla.org/integration/autoland/rev/012ce6437a4cbb2bdffb1d641dd08f0a4ca66b75

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a098226e42117f59f17112ed7be9e39710e18681&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=245716507&repo=autoland&lineNumber=33681

[task 2019-05-10T09:42:48.215Z] 09:42:48 INFO - make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/dom/ipc'
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -isysroot /builds/worker/workspace/build/src/MacOSX10.11.sdk --target=x86_64-darwin11 -o Unified_cpp_dom_ipc0.o -c -fvisibility=hidden -fvisibility-inlines-hidden -DDEBUG=1 -DOS_POSIX=1 -DOS_MACOSX=1 '-DBIN_SUFFIX=""' -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/dom/ipc -I/builds/worker/workspace/build/src/obj-firefox/dom/ipc -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/caps -I/builds/worker/workspace/build/src/chrome -I/builds/worker/workspace/build/src/docshell/base -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/dom/bindings -I/builds/worker/workspace/build/src/dom/events -I/builds/worker/workspace/build/src/dom/filesystem -I/builds/worker/workspace/build/src/dom/geolocation -I/builds/worker/workspace/build/src/dom/media/webspeech/synth/ipc -I/builds/worker/workspace/build/src/dom/security -I/builds/worker/workspace/build/src/dom/storage -I/builds/worker/workspace/build/src/extensions/permissions -I/builds/worker/workspace/build/src/extensions/spellcheck/src -I/builds/worker/workspace/build/src/gfx/2d -I/builds/worker/workspace/build/src/hal/sandbox -I/builds/worker/workspace/build/src/js/xpconnect/loader -I/builds/worker/workspace/build/src/js/xpconnect/src -I/builds/worker/workspace/build/src/layout/base -I/builds/worker/workspace/build/src/media/webrtc -I/builds/worker/workspace/build/src/netwerk/base -I/builds/worker/workspace/build/src/netwerk/protocol/http -I/builds/worker/workspace/build/src/toolkit/components/printingui/ipc -I/builds/worker/workspace/build/src/toolkit/crashreporter -I/builds/worker/workspace/build/src/toolkit/xre -I/builds/worker/workspace/build/src/uriloader/exthandler -I/builds/worker/workspace/build/src/widget -I/builds/worker/workspace/build/src/xpcom/base -I/builds/worker/workspace/build/src/xpcom/threads -I/builds/worker/workspace/build/src/modules/libjar -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -Qunused-arguments -Wall -Wbitfield-enum-conversion -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wtautological-unsigned-enum-zero-compare -Wtautological-unsigned-zero-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=backend-plugin -Wno-error=return-std-move -Wno-error=atomic-alignment -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-sized-deallocation -fcrash-diagnostics-dir=/builds/worker/artifacts -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fno-exceptions -fno-strict-aliasing -stdlib=libc++ -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O3 -fno-omit-frame-pointer -funwind-tables -Werror -Wno-error=shadow -MD -MP -MF .deps/Unified_cpp_dom_ipc0.o.pp /builds/worker/workspace/build/src/obj-firefox/dom/ipc/Unified_cpp_dom_ipc0.cpp
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/ipc/Unified_cpp_dom_ipc0.cpp:119:
[task 2019-05-10T09:42:48.218Z] 09:42:48 ERROR - /builds/worker/workspace/build/src/dom/ipc/JSWindowActor.cpp:64:7: error: arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). 'callbacksHolder.mWillDestroy.Value()' is neither.
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - callbacksHolder.mWillDestroy.Value()->Call();
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2019-05-10T09:42:48.218Z] 09:42:48 ERROR - /builds/worker/workspace/build/src/dom/ipc/JSWindowActor.cpp:64:7: error: functions marked as MOZ_CAN_RUN_SCRIPT can only be called from functions also marked as MOZ_CAN_RUN_SCRIPT
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - callbacksHolder.mWillDestroy.Value()->Call();
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/JSWindowActor.h:79:8: note: caller function declared here
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - void DestroyCallback(DestroyCallbackFunction willDestroy);
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - ^
[task 2019-05-10T09:42:48.218Z] 09:42:48 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dom/ipc/Unified_cpp_dom_ipc0.cpp:119:
[task 2019-05-10T09:42:48.219Z] 09:42:48 ERROR - /builds/worker/workspace/build/src/dom/ipc/JSWindowActor.cpp:68:7: error: arguments must all be strong refs or caller's parameters when calling a function marked as MOZ_CAN_RUN_SCRIPT (including the implicit object argument). 'callbacksHolder.mDidDestroy.Value()' is neither.
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - callbacksHolder.mDidDestroy.Value()->Call();
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2019-05-10T09:42:48.219Z] 09:42:48 ERROR - /builds/worker/workspace/build/src/dom/ipc/JSWindowActor.cpp:68:7: error: functions marked as MOZ_CAN_RUN_SCRIPT can only be called from functions also marked as MOZ_CAN_RUN_SCRIPT
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - callbacksHolder.mDidDestroy.Value()->Call();
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/JSWindowActor.h:79:8: note: caller function declared here
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - void DestroyCallback(DestroyCallbackFunction willDestroy);
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - ^
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - 4 errors generated.
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - /builds/worker/workspace/build/src/config/rules.mk:835: recipe for target 'Unified_cpp_dom_ipc0.o' failed
[task 2019-05-10T09:42:48.219Z] 09:42:48 ERROR - make[4]: *** [Unified_cpp_dom_ipc0.o] Error 1
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/dom/ipc'
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'dom/ipc/target' failed
[task 2019-05-10T09:42:48.219Z] 09:42:48 ERROR - make[3]: *** [dom/ipc/target] Error 2
[task 2019-05-10T09:42:48.219Z] 09:42:48 INFO - make[3]: *** Waiting for unfinished jobs....

Flags: needinfo?(jdai)

I'll take a look. Sorry for any inconvenience.

Flags: needinfo?(jdai)
Pushed by jdai@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c30426096b27
Part 1: Clear mManager in ActorDestroy and disallow sending message while Destroying; r=nika
https://hg.mozilla.org/integration/autoland/rev/0e3d9d0c3260
Part 2: Add WillDestroy and DidDestroy lifecycle methods on JSWindowActor; r=nika
https://hg.mozilla.org/integration/autoland/rev/849fff1c1663
Part 3: Add testcase to test JSWindowActor's lifecycle; r=nika
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Depends on: 1557448
You need to log in before you can comment on or make changes to this bug.