581685 - We're _still_ flushing layout before firing onload

Status: NEW

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It turns out we actually had two different places that do such a flush.  I only fixed one in bug 572604.

Patch coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

[Boris Zbarsky [:bzbarsky]]

That patch causes some test failures.  Mostly bugs in the tests, but also one core issue and I'm still sorting out a few of them.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK.  So I'm still sorting through the failures, but one source of them is crashtests that stop asserting with this patch.  Presumably they exercise bugs in dynamic updates and assume that layout is flushed by onload....

I can fix this by just flushing layout in the reftest onload handler for crashtests, but I was considering doing it for all reftests in general in case some other tests also depend on that behavior for detecting the bugs they're testing.

The main drawback is that if I do that reftests won't run in the same situation as on the web, but I think it's ok... and that in particular this would not interfere with writing any tests, offhand.

Thoughts?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Presumably drawWindow already flushes layout, so the only thing that adding a layout flush to the reftest harness would affect is crashtests, right?  I think it probably makes sense to add such a flush.  (Maybe we could add the flush *after* onload, but before moving on to the next test?)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, except you want to add the flush before hitting the tests' onload handlers.  I guess that seems a little sketchy to me, but there certainly could be a lot of tests that depended on it, and we wouldn't want to lose that test coverage.

It feels like the "right" thing might be to modify all existing tests that have onload handlers, but that also seems like a lot of work.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Another option is for the harness to run each test twice: once with a flush before firing the page's onload, and once without.

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 460048 [details] [diff] [review]
Like so

Sorry, it's too late for 2.0.

Comment 8

[(no longer active)]

This is no longer just breaking crashtests! https://tbpl.mozilla.org/?tree=Try&rev=e21b19c06e53

Comment 9

[(no longer active)]

It seems like quite a few of these test failures are caused by XUL documents.  Can we special case this for only HTML documents?  (That would require to access the mDocument pointer which is currently checked after we flush, so I guess the behavior would change for documents that have a null mDocument pointer in their content viewer, not sure if we should worry about that or not.)

Comment 10

[(no longer active)]

Also, do we have any idea what we want to do for crashtests here?  There are currently two crashtests that currently fail with this patch on try, but I'm not sure if there are others which will be testing the wrong thing with this flush being removed.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Can we special case this for only HTML documents? 

We could, yeah.  Or at least non-XUL.

> so I guess the behavior would change for documents that have a null mDocument pointer in
> their content viewer

Null mDocument means we're torn down.  Which is something the onload scripts can do, hence checking that pointer after firing onload.  Not flushing if !mDocument is totally fine.

> but I'm not sure if there are others which will be testing the wrong thing with this
> flush being removed.

See comment 3.  :(

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 13

[Boris Zbarsky [:bzbarsky]]

Clearly not going to get to this. Back into the pool. It might still be nice to do something here if we still have this problem.