1568673 - Add a few more assertions to validate that mobile viewport zoom factors are positive

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, task, P3)

Description

[Daniel Holbert [:dholbert]]

Following up from bug 1566991 a bit: it seems that it's possible for the *newZoom > 0 assertion to fail, as noted in bug 1568405.

So let's add a few more assertions to see which clause is giving us a zero (or negative) newZoom value.

Comment 1

[Daniel Holbert [:dholbert]]

I'm planning on adding an assertion to ClampZoom() here.

That should give us coverage, because here are all the places where we give newZoom a value:
(1) newZoom = Some(defaultZoom);
... and here, the defaultZoom value comes from either:

• a ClampZoom(...) call (and I'm adding an assertion inside that function)
• or from an aViewportInfo.GetDefaultZoom() call (which is already checked via an assertion from Bug 1566991)
• or from intrinsicScale whose value comes from ComputeIntrinsicScale(...) which calls ClampZoom(...) before returning (and I'm adding an assertion to ClampZoom() as noted above.

(2) newZoom = Some(ClampZoom(adjustedZoom, aViewportInfo));
... and here, this will be trivially checked via the assertion I'm adding in ClampZoom.

(3) newZoom = Some(intrinsicScale);
...and intrinsicScale will be indirectly checked via my new ClampZoom assertion as discussed in a bullet point under (1) above.

(4) newZoom = Some(clampedZoom);
...where clampedZoom is the result of a ClampZoom() call and hence will be checked via my new assertion.

Comment 2

[Daniel Holbert [:dholbert]]

I don't immediately see how the assertion in bug 1568405 could fail, since I think a failure would imply that one of our ClampZoom calls isn't successfully clamping us to a positive range, which means we've got a nsViewportInfo with a min or max value that is negative or zero... And I don't think that's supposed to be possible based on my nsViewportInfo::ConstrainViewportValues() assertion in Bug 1566991.

But maybe there's a codepath I'm not considering where one of the constraints gets modified. Anyway, we can hopefully get more information once ClampZoom is stricter with assertions.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1568673 part 2: Add a few more assertions to validate that mobile viewport zoom factors are positive. r?botond

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70404a86aea8
Add a few more assertions to validate that mobile viewport zoom factors are positive. r=botond

Comment 5

[Daniel Varga [:dvarga]]

Backed out changeset 70404a86aea8 (Bug 1568673) for gv-unit6 failure at org.mozilla.geckoview.test.MediaElementTest.webmFullscreenMedia

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=258219166&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=70404a86aea865aa1f55f4d25dbd35877a956a26

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=258219166&repo=autoland&lineNumber=1613

Backout push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=258219166&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=61478865db53b5dd9f415f52c3d0a00ac0ba7ee2

[task 2019-07-25T01:33:33.506Z] 01:33:33     INFO -  org.mozilla.geckoview.test | 	at android.app.ActivityThread.main(ActivityThread.java:5103)
[task 2019-07-25T01:33:33.506Z] 01:33:33     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invokeNative(Native Method)
[task 2019-07-25T01:33:33.507Z] 01:33:33     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invoke(Method.java:525)
[task 2019-07-25T01:33:33.507Z] 01:33:33     INFO -  org.mozilla.geckoview.test | 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
[task 2019-07-25T01:33:33.507Z] 01:33:33     INFO -  org.mozilla.geckoview.test | 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
[task 2019-07-25T01:33:33.508Z] 01:33:33     INFO -  org.mozilla.geckoview.test | 	at dalvik.system.NativeStart.main(Native Method)
[task 2019-07-25T01:33:33.508Z] 01:33:33     INFO -  org.mozilla.geckoview.test |
[task 2019-07-25T01:33:33.509Z] 01:33:33     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=webmFullscreenMedia
[task 2019-07-25T01:33:33.509Z] 01:33:33     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: -2
[task 2019-07-25T01:33:33.509Z] 01:33:33  WARNING -  TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.MediaElementTest.webmFullscreenMedia | status -2
[task 2019-07-25T01:33:33.510Z] 01:33:33     INFO -  TEST-INFO took 146637ms
[task 2019-07-25T01:33:33.510Z] 01:33:33     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: id=AndroidJUnitRunner
[task 2019-07-25T01:33:33.511Z] 01:33:33     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: current=12
[task 2019-07-25T01:33:33.511Z] 01:33:33     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: class=org.mozilla.geckoview.test.MediaElementTest
[task 2019-07-25T01:33:33.512Z] 01:33:33     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stream=
[task 2019-07-25T01:33:33.512Z] 01:33:33     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: numtests=39
[task 2019-07-25T01:33:33.512Z] 01:33:33     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=mp4MetadataMedia
[task 2019-07-25T01:33:33.513Z] 01:33:33     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: 1
[task 2019-07-25T01:33:33.513Z] 01:33:33     INFO -  TEST-START | org.mozilla.geckoview.test.MediaElementTest.mp4MetadataMedia
[task 2019-07-25T01:33:54.538Z] 01:33:54     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: id=AndroidJUnitRunner
[task 2019-07-25T01:33:54.538Z] 01:33:54     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: current=12
[task 2019-07-25T01:33:54.538Z] 01:33:54     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: class=org.mozilla.geckoview.test.MediaElementTest
[task 2019-07-25T01:33:54.539Z] 01:33:54     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stream=
[task 2019-07-25T01:33:54.539Z] 01:33:54     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: numtests=39
[task 2019-07-25T01:33:54.539Z] 01:33:54     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stack=org.junit.AssumptionViolatedException: got: <true>, expected: <false>
[task 2019-07-25T01:33:54.540Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at org.junit.Assume.assumeThat(Assume.java:95)
[task 2019-07-25T01:33:54.540Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.MediaElementTest.mp4MetadataMedia(MediaElementTest.kt:360)
[task 2019-07-25T01:33:54.541Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invokeNative(Native Method)
[task 2019-07-25T01:33:54.542Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invoke(Method.java:525)
[task 2019-07-25T01:33:54.543Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
[task 2019-07-25T01:33:54.544Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
[task 2019-07-25T01:33:54.544Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
[task 2019-07-25T01:33:54.544Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
[task 2019-07-25T01:33:54.544Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule$2.lambda$evaluate$0$GeckoSessionTestRule$2(GeckoSessionTestRule.java:1261)
[task 2019-07-25T01:33:54.545Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.-$$Lambda$GeckoSessionTestRule$2$mzZNnl5Bu5F2_4xGxj0DHU4J33I.run(lambda)
[task 2019-07-25T01:33:54.545Z] 01:33:54     INFO -  org.mozilla.geckoview.test | 	at android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1719)

Comment 6

[Daniel Holbert [:dholbert]]

Aha, thanks! Looks like the failure is reliable, which is good news - I can hopefully get to the bottom of this tomorrow with some Try runs or emulator testruns.

Comment 7

[Daniel Holbert [:dholbert]]

So the assertion-failure here means we're getting through this code without actually clamping -- i.e. zoom > 0 is failing, despite the fact that both of the clamp boundaries are > 0:

CSSToScreenScale MobileViewportManager::ClampZoom(
    const CSSToScreenScale& aZoom, const nsViewportInfo& aViewportInfo) const {
  CSSToScreenScale zoom = aZoom;
  if (zoom < aViewportInfo.GetMinZoom()) {
    zoom = aViewportInfo.GetMinZoom();
    MVM_LOG("%p: Clamped to %f\n", this, zoom.scale);
  }
  if (zoom > aViewportInfo.GetMaxZoom()) {
    zoom = aViewportInfo.GetMaxZoom();
    MVM_LOG("%p: Clamped to %f\n", this, zoom.scale);
  }
  return zoom;
}

This can only happen if zoom is NaN, which returns "false" for all comparisons. So, it's "not less" than our minimum, and "not more" than our maximum -- hence, no clamping -- but it's also "not more than zero", hence the assertion failure in the assertion added in this patch.

Comment 8

[Daniel Holbert [:dholbert]]

Based on the failure log, the caller here is ComputeIntrinsicScale(), and it passes in the result of a MaxScaleRatio() expression:
https://searchfox.org/mozilla-central/rev/1dfd70469212ef2785d41827c5532c571c784227/layout/base/MobileViewportManager.cpp#90-97

MaxScaleRatio is just defined as a division, basically -- so it could totally do a divide-by-zero and produce NaN.

We should probably add a special case in MaxScaleRatio() (and its friend MinScaleRatio) to check for zero in the denominator, and just return 0 instead of NaN so that e.g. clamping will produce sane behavior. botond, does that sound OK?

Comment 9

[Daniel Holbert [:dholbert]]

Alternately, we could change the callsite in ComputeIntrinsicScale to test for zero-sized area before it calls MaxScaleRatio. Perhaps different callsites need/expect different behavior.

Comment 10

[Daniel Holbert [:dholbert]]

Oh, I guess this is the only caller of MaxScaleRatio :) And there are no callers of MinScaleRatio.

I'll put the bail-out code in the caller, then, and add an assertion to Min/MaxScaleRatio to enforce that callers should check the divisor for 0 sizes.

Comment 11

[Botond Ballo [:botond]]

Sounds good to me! Thanks for tracking this down.

We should probably use 1.0 as the fallback value for any scale, to prevent possible further divisions-by-zero downstream.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: Bug 1568673 part 1: Adjust MobileViewportManager::ComputeIntrinsicScale to prevent divide-by-zero and to allow for meaningful min/max clamping. r?botond

Comment 13

[Daniel Holbert [:dholbert]]

Ah, just saw comment 11. I'll adjust part 1 to use 1.0 instead of 0.0-which-expects-to-be-clamped-to-min.

Comment 14

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5995626507595fd27d359b7552efaaacf3e33a86&selectedJob=258603853

Some orange, but I think it's all intermittent/unrelated since this patch stack is basically just adding assertions (and changing one divide-by-0 to something more predictable, which is unlikely to impact test behavior).

I'll go ahead and trigger autoland.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15a6fc165bdb
part 1: Adjust MobileViewportManager::ComputeIntrinsicScale to prevent divide-by-zero and to allow for meaningful min/max clamping. r=botond
https://hg.mozilla.org/integration/autoland/rev/8007ff804e99
part 2: Add a few more assertions to validate that mobile viewport zoom factors are positive. r=botond

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/15a6fc165bdb
https://hg.mozilla.org/mozilla-central/rev/8007ff804e99