Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines. #13387

emilio · 2016-09-23T06:29:59Z

As part of #13127.

cc @aneeshusa

./mach build -d does not report any errors
./mach test-tidy does not report any errors

This change is

highfive · 2016-09-23T06:30:04Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/build_commands.py

emilio · 2016-09-23T06:30:35Z

@bors-servo: try

Not sure if the new build config has been rolled out already.

bors-servo · 2016-09-23T06:30:37Z

⌛ Trying commit 16771bc with merge de58d53...

@aneeshusa

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines.  As part of #13127. cc @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13387)

bors-servo · 2016-09-23T06:30:54Z

💔 Test failed - arm64

Ms2ger · 2016-09-23T09:25:58Z

Do we really want an env var for this? Arguments to mach are easier to document and discover.

emilio · 2016-09-23T09:36:03Z

@bors-servo: try

bors-servo · 2016-09-23T09:36:06Z

⌛ Trying commit 8266e40 with merge 4aa4372...

@aneeshusa

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines.  As part of #13127. cc @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13387)

emilio · 2016-09-23T09:37:45Z

@Ms2ger: You're right, see the TODO.

The reason why this is done this way is to prevent shipping a change in saltfs that could potentially cause a lot of perma-failures. We added the env var first, which is a no-op, then see the result of running the tests with debug assertions in another servo PR (this one).

bors-servo · 2016-09-23T10:55:03Z

💔 Test failed - linux-rel

emilio · 2016-09-23T11:00:51Z

@bors-servo: retry (build was interrupted)

bors-servo · 2016-09-23T11:00:54Z

⌛ Trying commit 8266e40 with merge 7fab83e...

@aneeshusa

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines.  As part of #13127. cc @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13387)

bors-servo · 2016-09-23T11:53:01Z

💔 Test failed - linux-rel

highfive · 2016-09-23T11:53:04Z

  ▶ Unexpected subtest result in /_mozilla/mozilla/promise.html:
  │ FAIL [expected PASS] Native promise from async callback can be resolved
  │   → assert_true: expected true got false
  │ 
  └ @http://web-platform.test:8000/_mozilla/mozilla/promise.html:58:11

emilio · 2016-09-23T13:59:42Z

@bors-servo: try

bors-servo · 2016-09-23T13:59:44Z

⌛ Trying commit 537af57 with merge f147e4a...

@aneeshusa

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines.  As part of #13127. cc @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13387)

bors-servo · 2016-09-23T14:33:35Z

⛄ The build was interrupted to prioritize another pull request.

bors-servo · 2016-09-23T14:33:37Z

⌛ Trying commit 537af57 with merge e14d5a1...

@aneeshusa

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines.  As part of #13127. cc @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13387)

bors-servo · 2016-09-27T10:14:49Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

emilio · 2016-09-28T08:29:25Z

@bors-servo: retry

Try with the new renderer, just in case.

bors-servo · 2016-09-28T08:29:28Z

⌛ Trying commit 5d0c804 with merge 55334f7...

@aneeshusa

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines.  As part of #13127. cc @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13387)

bors-servo · 2016-09-28T18:37:51Z

💥 Test timed out

jdm · 2016-09-28T21:56:13Z

@bors-servo: retry

bors-servo · 2016-09-28T21:56:16Z

⌛ Trying commit 5d0c804 with merge 4895d91...

@aneeshusa

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines.  As part of #13127. cc @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13387)

bors-servo · 2016-09-28T23:23:11Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

emilio · 2016-10-02T12:21:53Z

review ping @jdm / @Ms2ger for the image setter bits and and @pcwalton / @mbrubeck for the writing mode changes?

jdm · 2016-10-02T15:35:16Z

components/script/dom/htmlimageelement.rs

@@ -435,14 +435,24 @@ impl VirtualMethods for HTMLImageElement {
 }

 fn image_dimension_setter(element: &Element, attr: Atom, value: u32) {
+    use app_units::AU_PER_PX;
+    use std::i32;


Can we move these to the top of the file?

jdm · 2016-10-02T15:45:36Z

components/script/dom/htmlimageelement.rs

    // This setter is a bit weird: the IDL type is unsigned long, but it's parsed as
    // a dimension for rendering.
    let value = if value > UNSIGNED_LONG_MAX {
        0
    } else {
        value
    };
-    let dim = LengthOrPercentageOrAuto::Length(Au::from_px(value as i32));
+
+    let pixel_value = if value > (i32::MAX / AU_PER_PX) as u32 {


I wonder if this belongs in Au::from_px, or whether Au::from_px should return an Option value instead? I don't think that I'm a good person to make that call.

Yeah, I'll open an issue for this in app_units, and mention it from here.

I'd still like to land this before that is resolved, since that's potentially a big change, and I think the benefits of this are worth it.

Filled servo/app_units#22

… to that fragment. Otherwise we might mix writing modes. Not totally sure this change is correct in the case we're mixing them, we might need to just not checking that operation.

…lement.rs

emilio · 2016-10-04T18:47:27Z

@bors-servo: r=aneeshusa,jdm,pcwalton

20:43 <•pcwalton> emilio: looks good
20:43 <•pcwalton> r=me if you need it
20:43 <emilio> jdm: do the changes to the image setter bits look ok to you?
20:44 <•jdm> emilio: link me again so I can refresh my memory?
20:44 <emilio> jdm: https://github.com/servo/servo/pull/13387/commits/fdd84713105b5c87f7adde1319711966d00e5b66
20:45 <•jdm> emilio: yes

Finally!

bors-servo · 2016-10-04T18:47:28Z

📌 Commit fdd8471 has been approved by aneeshusa,jdm,pcwalton

emilio · 2016-10-04T18:47:48Z

(Thanks for the reviews btw!)

bors-servo · 2016-10-04T19:01:50Z

⌛ Testing commit fdd8471 with merge 19a5a30...

@aneeshusa

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines.  As part of #13127. cc @aneeshusa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13387)

bors-servo · 2016-10-04T20:06:16Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned jdm Sep 23, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 23, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 23, 2016

emilio force-pushed the debug-assertions branch from 16771bc to 8266e40 Compare September 23, 2016 09:31

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 23, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 23, 2016

highfive removed the S-tests-failed The changes caused existing tests to fail. label Sep 23, 2016

emilio removed the S-tests-failed The changes caused existing tests to fail. label Sep 28, 2016

emilio mentioned this pull request Oct 2, 2016

Fix WebGL tests & Implement WebGLRenderingContext::{validateProgram, getProgramInfoLog, disableVertexAttribArray}. #13208

Merged

5 tasks

jdm requested changes Oct 2, 2016

View reviewed changes

emilio added 3 commits October 2, 2016 17:57

build: Honor SERVO_ENABLE_DEBUG_ASSERTIONS.

6496efd

script: Fix bogus is_listed_element assertion.

3dddb70

layout: Use the fragment's writing mode when computing sizes relevant…

bdf1d17

… to that fragment. Otherwise we might mix writing modes. Not totally sure this change is correct in the case we're mixing them, we might need to just not checking that operation.

emilio mentioned this pull request Oct 2, 2016

Au::from_px can overflow. servo/app_units#22

Closed

script: Prevent an integer overflow that was hitting us on htmlimagee…

fdd8471

…lement.rs

emilio force-pushed the debug-assertions branch from 5d0c804 to fdd8471 Compare October 2, 2016 16:02

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 4, 2016

bors-servo merged commit fdd8471 into servo:master Oct 4, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 4, 2016

aneeshusa mentioned this pull request Nov 1, 2016

Add a mach argument to enable debug assertions #14009

Closed

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines. #13387

Honor SERVO_ENABLE_DEBUG_ASSERTIONS on the build machines. #13387

Conversation

emilio commented Sep 23, 2016 • edited by larsbergstrom

highfive commented Sep 23, 2016

emilio commented Sep 23, 2016

bors-servo commented Sep 23, 2016

bors-servo commented Sep 23, 2016

Ms2ger commented Sep 23, 2016

emilio commented Sep 23, 2016

bors-servo commented Sep 23, 2016

emilio commented Sep 23, 2016

bors-servo commented Sep 23, 2016

emilio commented Sep 23, 2016

bors-servo commented Sep 23, 2016

bors-servo commented Sep 23, 2016

highfive commented Sep 23, 2016

emilio commented Sep 23, 2016

bors-servo commented Sep 23, 2016

bors-servo commented Sep 23, 2016

bors-servo commented Sep 23, 2016

bors-servo commented Sep 27, 2016

emilio commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

jdm commented Sep 28, 2016

bors-servo commented Sep 28, 2016

bors-servo commented Sep 28, 2016

emilio commented Oct 2, 2016 • edited

jdm Oct 2, 2016

Choose a reason for hiding this comment

emilio Oct 2, 2016

Choose a reason for hiding this comment

jdm Oct 2, 2016 • edited

Choose a reason for hiding this comment

emilio Oct 2, 2016

Choose a reason for hiding this comment

emilio Oct 2, 2016

Choose a reason for hiding this comment

emilio commented Oct 4, 2016

bors-servo commented Oct 4, 2016

emilio commented Oct 4, 2016

bors-servo commented Oct 4, 2016

bors-servo commented Oct 4, 2016

emilio commented Sep 23, 2016 •

edited by larsbergstrom

emilio commented Oct 2, 2016 •

edited

jdm Oct 2, 2016 •

edited