Send IPC receiver for canvas as part of CreateCanvasPaintThread message #19547

tigercosmos · 2017-12-12T07:21:41Z

I am not sure if @jdm want this.
r? @jdm

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #19483(github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-12-12T07:21:45Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script_traits/script_msg.rs, components/constellation/constellation.rs, components/script/dom/canvasrenderingcontext2d.rs
@cbrewster: components/constellation/constellation.rs
@paulrouget: components/constellation/constellation.rs
@fitzgen: components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/canvasrenderingcontext2d.rs
@KiChjang: components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/canvasrenderingcontext2d.rs

highfive · 2017-12-12T07:21:46Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

tigercosmos · 2017-12-14T06:50:03Z

@jdm Could you check this?

jdm

Good start, but we want to remove the synchronous communication entirely.

jdm · 2017-12-14T16:14:29Z

components/canvas/canvas_paint_thread.rs

@@ -118,9 +118,10 @@ impl<'a> CanvasPaintThread<'a> {
    /// communicate with it.
    pub fn start(size: Size2D<i32>,
                 webrender_api_sender: webrender_api::RenderApiSender,
-                 antialias: bool)
+                 antialias: bool,
+                 canvas_channel: (IpcSender<CanvasMsg>, IpcReceiver<CanvasMsg>))
                 -> IpcSender<CanvasMsg> {


Since all this method does is return the sender, we can make this code more efficient by only accepting the IpcReceiver argument and not returning anything.

jdm · 2017-12-14T16:14:29Z

components/script_traits/script_msg.rs

@@ -79,7 +79,8 @@ pub enum ScriptMsg {
    ChangeRunningAnimationsState(AnimationState),
    /// Requests that a new 2D canvas thread be created. (This is done in the constellation because
    /// 2D canvases may use the GPU and we don't want to give untrusted content access to the GPU.)
-    CreateCanvasPaintThread(Size2D<i32>, IpcSender<IpcSender<CanvasMsg>>),
+    CreateCanvasPaintThread(
+        Size2D<i32>, IpcSender<IpcSender<CanvasMsg>>, (IpcSender<CanvasMsg>, IpcReceiver<CanvasMsg>)),


We should be able to remove the IpcSender<IpcSender> argument and only pass the IpcReceiver argument instead.

jdm · 2017-12-14T16:14:29Z

components/script/dom/canvasrenderingcontext2d.rs

@@ -130,9 +130,10 @@ impl CanvasRenderingContext2D {
                         -> CanvasRenderingContext2D {
        debug!("Creating new canvas rendering context.");
        let (sender, receiver) = ipc::channel().unwrap();
+        let canvas_channel = ipc::channel::<CanvasMsg>().unwrap();


We should be able to do let (sender, ipc_renderer) = ipc::channel::<CanvasMsg>().unwrap() and remove the let ipc_renderer and let (sender, receiver) lines entirely.

tigercosmos · 2017-12-15T05:53:48Z

@jdm I have fixed them. Please check.

tigercosmos · 2017-12-16T04:04:47Z

@jdm Fixed. Please check.


        Send IPC receiver for canvas as part of CreateCanvasPaintThread message

jdm · 2017-12-16T15:04:12Z

@bors-servo r+
Thanks for fixing this!

bors-servo · 2017-12-16T15:04:13Z

📌 Commit a504c93 has been approved by jdm

bors-servo · 2017-12-16T15:04:17Z

⌛ Testing commit a504c93 with merge da9c3a5...


        Auto merge of #19547 - tigercosmos:b1, r=jdm

@jdm

Send IPC receiver for canvas as part of CreateCanvasPaintThread message  I am not sure if @jdm want this. r? @jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #19483(github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19547)

bors-servo · 2017-12-16T15:13:46Z

💔 Test failed - mac-dev-unit

tigercosmos · 2017-12-16T15:51:05Z

@jdm what happens?

error: failed to run custom build command for `gecko-media v0.1.0 (https://github.com/servo/gecko-media.git#fe437442)`
process didn't exit successfully: `/Users/servo/buildbot/slave/mac-dev-unit/build/target/debug/build/gecko-media-8ac78880f09277b6/build-script-build` (exit code: 101)

---

error: Connection to server timed out
make[2]: *** [CMakeFiles/ogg.dir/gecko/src/media/libogg/src/ogg_alloc.c.o] Error 2
make[1]: *** [CMakeFiles/ogg.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/test.cpp:223:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  aQueue->Dispatch(r.forget());
  ^~~~~~~~~~~~~~~~
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/test.cpp:223:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  aQueue->Dispatch(r.forget());
  ^~~~~~~~~~~~~~~~
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/test.cpp:257:3: note: in instantiation of function template specialization 'GeckoMedia_mozilla::RunOnTaskQueue<(lambda at /Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/test.cpp:257:25)>' requested here
  RunOnTaskQueue(queue, [queue]() -> void {
  ^
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/test.cpp:223:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
  aQueue->Dispatch(r.forget());
  ^~~~~~~~~~~~~~~~
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/test.cpp:268:3: note: in instantiation of function template specialization 'GeckoMedia_mozilla::RunOnTaskQueue<(lambda at /Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/test.cpp:268:25)>' requested here
  RunOnTaskQueue(queue, [queue]() -> void {
  ^
3 warnings generated.
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/TestMP4Demuxer.cpp:157:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    mTaskQueue->Dispatch(r.forget());
    ^~~~~~~~~~~~~~~~~~~~
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/TestMP4Demuxer.cpp:157:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    mTaskQueue->Dispatch(r.forget());
    ^~~~~~~~~~~~~~~~~~~~
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/TestMP4Demuxer.cpp:88:5: note: in instantiation of function template specialization 'GeckoMedia_mozilla::MP4DemuxerBinding::DispatchTask<(lambda at /Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/TestMP4Demuxer.cpp:89:7)>' requested here
    DispatchTask(
    ^
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/TestMP4Demuxer.cpp:157:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    mTaskQueue->Dispatch(r.forget());
    ^~~~~~~~~~~~~~~~~~~~
/Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/TestMP4Demuxer.cpp:118:5: note: in instantiation of function template specialization 'GeckoMedia_mozilla::MP4DemuxerBinding::DispatchTask<(lambda at /Users/servo/.cargo/git/checkouts/gecko-media-6bedb392a81eb7cf/fe43744/gecko-media/gecko/test/TestMP4Demuxer.cpp:119:7)>' requested here
    DispatchTask(
    ^
3 warnings generated.
make: *** [all] Error 2
thread 'main' panicked at '
command did not execute successfully, got: exit code: 2

build script failed, must exit now', /Users/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/cmake-0.1.27/src/lib.rs:627:4
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
   1: std::sys_common::backtrace::print
   2: std::panicking::default_hook::{{closure}}
   3: std::panicking::default_hook
   4: std::panicking::rust_panic_with_hook
   5: std::panicking::begin_panic
   6: std::panicking::begin_panic_fmt
   7: cmake::fail
   8: cmake::run
   9: cmake::Config::build
  10: cmake::build
  11: build_script_build::compile_gecko_media
  12: build_script_build::main
  13: __rust_maybe_catch_panic
  14: std::rt::lang_start
  15: main

warning: build failed, waiting for other jobs to finish...
error: build failed
[Warning] Could not generate notification! Optional Python module 'pyobjc' is not installed.
Build FAILED in 0:08:56
program finished with exit code 101
elapsedTime=536.361900

emilio · 2017-12-16T15:57:51Z

@bors-servo retry

Known intermittent.

tigercosmos · 2017-12-16T16:19:45Z

@emilio Seems it not work with retry?

bors-servo · 2017-12-16T17:35:42Z

⌛ Testing commit a504c93 with merge c9e3fab...


        Auto merge of #19547 - tigercosmos:b1, r=jdm

@jdm

Send IPC receiver for canvas as part of CreateCanvasPaintThread message  I am not sure if @jdm want this. r? @jdm ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #19483(github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19547)

bors-servo · 2017-12-16T18:19:01Z

💔 Test failed - linux-rel-wpt

tigercosmos · 2017-12-16T18:23:00Z

{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/2dcontext/line-styles/lineto_a.html", "line": 1748, "action": "test_result", "expected": "PASS"}
{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/_mozilla/css/canvas_linear_gradient_a.html", "line": 41738, "action": "test_result", "expected": "FAIL"}

tigercosmos · 2017-12-16T18:27:19Z

@bors-servo retry

bors-servo · 2017-12-16T18:27:22Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt2, mac-rel-wpt3, windows-msvc-dev are reusable. Rebuilding only linux-rel-wpt, mac-rel-wpt1, mac-rel-wpt4...

bors-servo · 2017-12-16T18:49:37Z

💔 Test failed - mac-rel-wpt4

tigercosmos · 2017-12-16T19:03:48Z

{"status": "TIMEOUT", "group": "default", "message": null, "stack": null, "subtest": null, "test": "/html/infrastructure/safe-passing-of-structured-data/shared-array-buffers/serialization-via-idb.any.worker.html", "line": 81480, "action": "test_result", "expected": "ERROR"}

tigercosmos · 2017-12-16T19:07:34Z

@bors-servo retry

bors-servo · 2017-12-16T19:07:36Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt4...

bors-servo · 2017-12-16T20:21:21Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing c9e3fab to master...


        Auto merge of #19599 - jdm:revertcanvas, r=asajeffrey

Revert canvas IPC changes #19547 is responsible for the surge of new intermittent timeouts in canvas-related tests. There's nothing wrong with the change, so I suspect an underlying problem in ipc-channel instead. Fixes #19592. #19593. Fixes #19594. Fixes #19597.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19599)


        servo: Merge #19599 - Revert canvas IPC changes (from jdm:revertcanva…

…s); r=asajeffrey servo/servo#19547 is responsible for the surge of new intermittent timeouts in canvas-related tests. There's nothing wrong with the change, so I suspect an underlying problem in ipc-channel instead. Fixes #19592. #19593. Fixes #19594. Fixes #19597. Source-Repo: https://github.com/servo/servo Source-Revision: 37fe9f29573e669ea9dd86ed6d9de0e43b3f746d --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 84f6a3819cb33421e270a2a8bbd6b62dcf0e9afb


        servo: Merge #19599 - Revert canvas IPC changes (from jdm:revertcanva…

…s); r=asajeffrey servo/servo#19547 is responsible for the surge of new intermittent timeouts in canvas-related tests. There's nothing wrong with the change, so I suspect an underlying problem in ipc-channel instead. Fixes #19592. #19593. Fixes #19594. Fixes #19597. Source-Repo: https://github.com/servo/servo Source-Revision: 37fe9f29573e669ea9dd86ed6d9de0e43b3f746d


        servo: Merge #19599 - Revert canvas IPC changes (from jdm:revertcanva…

…s); r=asajeffrey servo/servo#19547 is responsible for the surge of new intermittent timeouts in canvas-related tests. There's nothing wrong with the change, so I suspect an underlying problem in ipc-channel instead. Fixes #19592. #19593. Fixes #19594. Fixes #19597. Source-Repo: https://github.com/servo/servo Source-Revision: 37fe9f29573e669ea9dd86ed6d9de0e43b3f746d


        servo: Merge #19599 - Revert canvas IPC changes (from jdm:revertcanva…

…s); r=asajeffrey servo/servo#19547 is responsible for the surge of new intermittent timeouts in canvas-related tests. There's nothing wrong with the change, so I suspect an underlying problem in ipc-channel instead. Fixes #19592. #19593. Fixes #19594. Fixes #19597. Source-Repo: https://github.com/servo/servo Source-Revision: 37fe9f29573e669ea9dd86ed6d9de0e43b3f746d UltraBlame original commit: 6b1f9a0e13469a001e6ca2feda6a05153a4d9414


        servo: Merge #19599 - Revert canvas IPC changes (from jdm:revertcanva…

…s); r=asajeffrey servo/servo#19547 is responsible for the surge of new intermittent timeouts in canvas-related tests. There's nothing wrong with the change, so I suspect an underlying problem in ipc-channel instead. Fixes #19592. #19593. Fixes #19594. Fixes #19597. Source-Repo: https://github.com/servo/servo Source-Revision: 37fe9f29573e669ea9dd86ed6d9de0e43b3f746d UltraBlame original commit: 6b1f9a0e13469a001e6ca2feda6a05153a4d9414


        servo: Merge #19599 - Revert canvas IPC changes (from jdm:revertcanva…

…s); r=asajeffrey servo/servo#19547 is responsible for the surge of new intermittent timeouts in canvas-related tests. There's nothing wrong with the change, so I suspect an underlying problem in ipc-channel instead. Fixes #19592. #19593. Fixes #19594. Fixes #19597. Source-Repo: https://github.com/servo/servo Source-Revision: 37fe9f29573e669ea9dd86ed6d9de0e43b3f746d UltraBlame original commit: 6b1f9a0e13469a001e6ca2feda6a05153a4d9414

highfive assigned jdm Dec 12, 2017

highfive added the S-awaiting-review label Dec 12, 2017

jdm requested changes Dec 14, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Dec 14, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Dec 15, 2017

Send IPC receiver for canvas as part of CreateCanvasPaintThread message

Loading status checks…

a504c93

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 16, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 16, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 16, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 16, 2017

highfive added the S-tests-failed label Dec 16, 2017

bors-servo merged commit a504c93 into servo:master Dec 16, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

tigercosmos deleted the tigercosmos:b1 branch Dec 17, 2017

This was referenced Dec 18, 2017

Revert canvas IPC changes #19599

Merged

Send IPC receiver for canvas as part of CreateCanvasPaintThread message #19483

Open

servo / servo

Send IPC receiver for canvas as part of CreateCanvasPaintThread message #19547

Send IPC receiver for canvas as part of CreateCanvasPaintThread message #19547

tigercosmos commented Dec 12, 2017 •

edited by SimonSapin

highfive commented Dec 12, 2017

highfive commented Dec 12, 2017

tigercosmos commented Dec 14, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

tigercosmos commented Dec 15, 2017

tigercosmos commented Dec 16, 2017

jdm commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

tigercosmos commented Dec 16, 2017 •

edited

emilio commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

servo / servo

Join GitHub today

Send IPC receiver for canvas as part of CreateCanvasPaintThread message #19547

Send IPC receiver for canvas as part of CreateCanvasPaintThread message #19547

Conversation

tigercosmos commented Dec 12, 2017 • edited by SimonSapin

highfive commented Dec 12, 2017

highfive commented Dec 12, 2017

tigercosmos commented Dec 14, 2017

jdm left a comment

This comment has been minimized.

jdm Dec 14, 2017

This comment has been minimized.

jdm Dec 14, 2017

This comment has been minimized.

jdm Dec 14, 2017

tigercosmos commented Dec 15, 2017

tigercosmos commented Dec 16, 2017

jdm commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

tigercosmos commented Dec 16, 2017 • edited

emilio commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

tigercosmos commented Dec 16, 2017

bors-servo commented Dec 16, 2017

bors-servo commented Dec 16, 2017

tigercosmos commented Dec 12, 2017 •

edited by SimonSapin

tigercosmos commented Dec 16, 2017 •

edited