589442 - [tbplbot fails] Intermittent failure in test_suspend.html: wrong url, already suspended, already resumed over and over

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282397897.1282398760.29607.gz

s: talos-r3-fed64-030
3440 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/src/threads/test/test_suspend.html | Wrong url! - got http://mochi.test:8888/tests/dom/src/threads/test/suspend_iframe.html, expected "about:blank"
3441 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/src/threads/test/test_suspend.html | Already suspended? - got true, expected false
3442 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/src/threads/test/test_suspend.html | Already resumed? - got true, expected false

repeated a few hundred times.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Hit this on the 4.0b5 release tests today:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Release/1283266349.1283267231.18092.gz

Comment 2

[Phil Ringnalda (:philor)]

We also hit this several times a day on various non-release trees, but I hadn't noticed that tbpl was failing to actually comment when it's starred.

Comment 3

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283534622.1283535459.29728.gz

Comment 4

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283757952.1283758812.17181.gz

Comment 5

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1283764762.1283765591.16187.gz

Comment 6

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284158044.1284159641.25420.gz

Comment 7

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284181151.1284182002.30921.gz

Comment 8

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1284421741.1284422695.23529.gz

Comment 9

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1284425872.1284426805.10198.gz

Comment 10

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284491128.1284491977.10494.gz

Comment 11

[Justin Wood (:Callek)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284917252.1284918080.32484.gz

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1285062742.1285063616.1823.gz

Comment 13

[Daniel Holbert [:dholbert]]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1285440191.1285441031.21224.gz

Comment 14

[(no longer active)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1285497423.1285498299.31968.gz

Comment 15

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289439221.1289442771.3101.gz

Comment 16

[Markus Stange [:mstange]]

Attachment: add a flush

With my patch from bug 598482 I hit this failure on all platforms consistently.

The problem was that calling iframe.history.back() didn't restore test_suspend.html from bfcache, but reloaded it instead. (badOnloadCallback didn't catch the reload because it wasn't attached as a load handler correctly.)

Why didn't test_suspend.html come from the bfcache? Because an attribute change had kicked it out. This attribute change was caused by a delayed reflow flush. Without the patch from bug 598482, anything that causes invalidations causes reflows to be flushed pretty much instantly (one trip through the event loop). But with that patch, they are flushed from the refresh driver timer, and that can be too late.
When is "too late"? When the new document is shown, because that's when the content viewer of the previous document is stored on the session history entry. Any change to the stored document after that point will cause the entry to drop the cached content viewer.

(How can a reflow change attributes? For example through nsGfxScrollFrameInner::FinishReflowForScrollbar...)

Comment 17

[Markus Stange [:mstange]]

Maybe nsDocumentViewer should do the flush before it adds itself to the session history entry. Boris, do you think that's a good idea?

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 493291 [details] [diff] [review]
add a flush

Neat! Thanks for digging in to this!

Comment 19

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/4d4065d46e80

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Maybe nsDocumentViewer should do the flush before it adds itself

No, that seems like a terrible idea; it introduces another place script can run during the unload sequence, and does work we don't really need to do in the common case of not coming back to the page.

Are we really hitting a reflow from the refresh driver?  The refresh driver should be suspended while the page is in bfcache...  See nsRefreshDriver::Freeze.  Is that not working?

Also, perhaps we should special-case scrollbar attr changes in the shentry mutation observers and not drop bfcache for those.