986660 - [e10s] Lots of "ASSERTION: math on NS_UNCONSTRAINEDSIZE" during crashtests

Status: NEW

(Core :: Layout, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Here's a sample log:

https://tbpl.mozilla.org/php/getParsedLog.php?id=36479512&full=1&branch=holly#error1

This is what happens when you run crashtests-ipc in a debug build (with the remote tabs option set). I'm mostly hoping for some help with what this assertion means or how to debug it.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What's happening here is that in e10s mode we're calling SetCSSViewport:
18:50:46     INFO -  nsDOMWindowUtils::SetCSSViewport(float, float) [dom/base/nsDOMWindowUtils.cpp:281]
18:50:46     INFO -  mozilla::dom::TabChild::SetCSSViewport(mozilla::gfx::SizeTyped<mozilla::CSSPixel> const&) [dom/ipc/TabChild.cpp:499]
and that's triggering a resize reflow which doesn't happen in the non-e10s case, which is triggering those assertions.

We could downgrade that assertion to a warning, which is probably what would happen if this showed up in non-e10s, but there's actually a bigger problem here which is that those extra reflows are going to be a performance hit for e10s so we need to avoid them. In particular we should not be setting the viewport to kDefaultViewportSize ever for desktop e10s child documents, even if APZC gets enabled on desktop (problem 1). It also seems to be a problem that the page has been reflowed at a different size before this call to SetCSSViewport (problem 2).

It also seems to be a problem that e10s builds and tests run with APZC enabled. You don't really want to be tied to that. OTOH we want Ripc at least to run with APZC enabled to catch APZC bugs. Not sure what to do about that.

Anyway, focusing on problems 1 and 2 ... I think on desktop, when APZC is enabled, we should never use kViewportDefaultScreenWidth or kDefaultViewportSize (which should be defined in nsViewportInfo so that "980" isn't duplicated). Instead we should just use TabChild's mInnerSize. So I think we should have a pref that enables "desktop mode" for APZC instead of "mobile mode" and when that pref is set, make nsDocument::GetViewportInfo use aDisplaySize.width instead of "browser.viewport.desktopWidth" or kViewportDefaultScreenWidth. Likewise TabChild::Observe would pass mInnerSize to SetCSSViewport. mbrubeck, what do you think of that?

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> So I think we should have a pref that enables "desktop mode" for APZC
> instead of "mobile mode" and when that pref is set, make
> nsDocument::GetViewportInfo use aDisplaySize.width instead of
> "browser.viewport.desktopWidth" or kViewportDefaultScreenWidth. Likewise
> TabChild::Observe would pass mInnerSize to SetCSSViewport. mbrubeck, what do
> you think of that?

Yes, this sounds good to me.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: crashtest-assert

This doesn't even compile, but I'm hoping you can tell me what the right thing to do is.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Actually, I just looked at the APZC prefs. APZC is only enabled on desktop when running reftests (and only then when using OMTC and remote content) [1]. 

Right now we run a much abbreviated set of reftests in the reftest-ipc suite. As part of e10s testing, I'm trying to get the full set of reftests and crashtests running with remote tabs. Once this is done, I was planning to disable reftest-ipc and crashtest-ipc. However, there's no reason we couldn't keep these suites around to test APZC. They're pretty quick and they only run in Linux opt. Then we could disable APZC in normal e10s reftest runs, which is better anyway because APZC is not actually used by e10s on desktop.

To do this, I think we'd just need to change mozharness to pass an extra APZC pref to reftest-ipc and crashtest-ipc. Does that sound okay to you Rob?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-preferences.js#33

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's OK, although I still think we need your patch anyway.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> That's OK, although I still think we need your patch anyway.

OK, thanks. How do I convert the coordinates?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8400865 [details] [diff] [review]
crashtest-assert

Review of attachment 8400865 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabChild.cpp
@@ +738,5 @@
>  
> +        if (Preferences::GetBool("layers.async-pan-zoom.desktop-mode", false)) {
> +          // XXX doesn't work because mInnerSize is a ScreenIntSize and
> +          // SetCSSViewport takes a CSSSize
> +          SetCSSViewport(mInnerSize);

How about we just don't call SetCSSViewport at all for desktop APZC. I think that's probably the best.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I filed bug 991837 for changing when APZC is enabled.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: crashtest-assert v2

OK, here you go.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: crashtest-assert v3

Argh, forgot qref.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8401551 [details] [diff] [review]
crashtest-assert v3

Review of attachment 8401551 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: browser/app/profile/firefox.js
@@ +1419,5 @@
>  pref("experiments.manifest.certs.1.issuerName", "CN=Cybertrust Public SureServer SV CA,O=Cybertrust Inc");
> +
> +// This pref changes how the size of the viewport is determined for
> +// async pan zoom.
> +pref("layers.async-pan-zoom.desktop-mode", true);

We need a default value for this in all.js too ... setting it to false there, I guess.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hmm, this is causing content/base/test/test_meta_viewport0.html to fail. Maybe the change to nsDocument::GetViewportInfo needs to somehow be conditioned on APZC being enabled?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Maybe somebody else can take this over. Turning off APZC in the tests solved the immediate problem I was having.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.