1262432 - browser_device_width.js crashes at nsLayoutUtils::HasDisplayPort(content) on e10s debug

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P1)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This crash seems to be permanent.  We'll need to investigate this in more detail.  It's a bit surprising that it appears on only one platform, so it may be some kind of async race.

Example log:

https://treeherder.mozilla.org/logviewer.html#?job_id=19011621&repo=try

Comment 1

[Ryan VanderMeulen [:RyanVM]]

This is frequent on Linux and OSX as well on the Ash repo (where we run e10s tests across all platforms).

https://treeherder.mozilla.org/logviewer.html#?job_id=163545&repo=ash

Comment 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b175404c25d0

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b175404c25d0

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I caught this in rr, investigating.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Great, thanks for checking it out :kats!

Comment 6

[Kartikaya Gupta (email:[e-mail])]

So the assertion was added in bug 1208780, but I'm not sure that it's a valid assertion. It seems to assert that the first time we get a repaint request for a root scrollable, it will already have displayport margins set on it. That's true in a lot of cases, but not all. In particular, in this case, I'm seeing that the root content element is getting a scrollid via the code at [1], and that doesn't set any margins. The APZ sends a repaint request which is the first time that the element gets margins, and that causes the assertion to fail.

:tnikkel, do you recall why you added that assertion?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=62a953a61527#1759

Comment 7

[Kartikaya Gupta (email:[e-mail])]

It looks like bug 1208780 comment 24 through to comment 26 has some discussion on this. I think the assumption was incorrect (or maybe it has become incorrect since that code was written), and we should be ok to take out the assertion. The call to APZCCallbackHelper::SetDisplayPortMargins already handles the case where there is no displayport, and sets zero-margins on the ancestors. (Although the root frame shouldn't have any ancestors, so it should be a no-op).

Comment 8

[Timothy Nikkel (:tnikkel)]

(got mid-aired, but this is what I wrote)

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> :tnikkel, do you recall why you added that assertion?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp?rev=62a953a61527#1759

I think this is the link you mean

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp?rev=84f8d2e0106c#259


See
https://bugzilla.mozilla.org/show_bug.cgi?id=1208780#c24

If we create a displayport then we want to make sure every async scrollable ancestor also has a displayport (a zero margin one). In that bug I determined (wrongly it seems) that UpdateRootFrame couldn't create a displayport, only modify one. So I added that assert. If we can create a displayport there then we should call SetZeroMarginDisplayPortOnAsyncScrollableAncestors. But I guess that will have no effect because it is the root? So assert that?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tnikkel) from comment #8)
> (got mid-aired, but this is what I wrote)
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> > cpp?rev=62a953a61527#1759
> 
> I think this is the link you mean
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/
> APZCCallbackHelper.cpp?rev=84f8d2e0106c#259
> 

That's the link to the assertion. The link I had was where the content element gets a scrollId (therefore an APZ) without getting displayport margins.

> See
> https://bugzilla.mozilla.org/show_bug.cgi?id=1208780#c24
> 
> If we create a displayport then we want to make sure every async scrollable
> ancestor also has a displayport (a zero margin one). In that bug I
> determined (wrongly it seems) that UpdateRootFrame couldn't create a
> displayport, only modify one. So I added that assert.

Ok, makes sense.

> If we can create a
> displayport there then we should call
> SetZeroMarginDisplayPortOnAsyncScrollableAncestors.

This should already be happening via the call to APZCCallbackHelper::SetDisplayPortMargins.

> But I guess that will
> have no effect because it is the root? So assert that?

I'm not really sure what assertion can capture that? I'd rather just leave out the assertion and let the SetZeroMarginDisplayPortOnAsyncScrollableAncestors do its job if it's needed.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: MozReview Request: Bug 1262432 - Remove assertion that may be legitimately false sometimes. r?tnikkel

Review commit: https://reviewboard.mozilla.org/r/45829/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45829/

Comment 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8740536 [details]
MozReview Request: Bug 1262432 - Remove assertion that may be legitimately false sometimes. r?tnikkel

https://reviewboard.mozilla.org/r/45829/#review42427

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc0294f96c5

Comment 13

[Kartikaya Gupta (email:[e-mail])]

RyanVM, should I go ahead and re-enable the test right away, or do you want to let my fix merge to Ash and bake for a bit first?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

We have Win7 debug devtools test coverage on inbound, so go ahead and re-enable whenever.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c93227b825a4

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fc0294f96c5
https://hg.mozilla.org/mozilla-central/rev/c93227b825a4

Comment 17

[Marco Mucci [:MarcoM]]

Hi Ryan, will this bug need verification?

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Marco Mucci [:MarcoM] from comment #17)
> Hi Ryan, will this bug need verification?

No, we should know enough from just the automated test results.