1417251 - Have the reftest harness use nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE to load URIs differing only by hash

Status: REOPENED

(Testing :: Reftest, enhancement)

Description

[Jonathan Watt [:jwatt]]

Running the reftest harness with the pref reftest.compareRetainedDisplayLists enabled (that pref will be added in bug 1382427) results in a handful of timeouts. Specifically, the following files:

  layout/reftests/bugs/289480.html#top
  layout/reftests/bugs/539949-1.html#test2
  layout/reftests/scrolling/deferred-anchor.xhtml#d
  layout/reftests/scrolling/subpixel-1.html#d
  layout/reftests/svg/smil/anim-view-01.svg#view

It seems the reason for this is because we load the tests (once with the pref enabled, once disabled) and the second load occurs as an anchor navigation rather than a reload. Hence we never get a 'load' event.

The load of new URIs is triggered by the LoadURI function in reftest-content.js where we pass the flag CI.nsIWebNavigation.LOAD_FLAGS_NONE. We could instead pass CI.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE to force a hard reload.

There's also the flag LOAD_FLAGS_IS_REFRESH which on the surface looks more appropriate, but that flag is documented with the comment "we should probably deprecate and remove it".

https://dxr.mozilla.org/mozilla-central/rev/fc194660762d1b92e1679d860a8bf41116d0f54f/docshell/base/nsIWebNavigation.idl#95

That doesn't seem to be true [any more], but it probably doesn't matter which we use.

Comment 1

[Jonathan Watt [:jwatt]]

If necessary we could check whether gCurrentURL and the URL passed to LoadURI are equal, ignoring fragment identifiers, before using LOAD_FLAGS_BYPASS_CACHE.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

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

Comment on attachment 8928324 [details] [diff] [review]
patch

>+    // We use LOAD_FLAGS_BYPASS_CACHE here instead of LOAD_FLAGS_NONE so that

I'd drop the "instead of LOAD_FLAGS_NONE" since LOAD_FLAGS_NONE is just 0, whereas everything else is actual bits.


This seems OK to me, but I'd like bz to review as well if he's available (or if not, somebody else with a bit more docshell knowledge).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8928324 [details] [diff] [review]
patch

I don't see how LOAD_FLAGS_BYPASS_CACHE would skip the check for an anchor scroll.  I assume that you've tested this patch and it does work, right?  If so, can you point me to where LOAD_FLAGS_BYPASS_CACHE is relevant to anchor scrolling in the docshell code?

(On a side note, if/when this bug is fixed we should remove the skip-if(styloVGecko) annotation on these tests, because it's working around precisely this anchor scroll issue.)

Comment 5

[Jonathan Watt [:jwatt]]

Both LOAD_FLAGS_BYPASS_CACHE and LOAD_FLAGS_IS_REFRESH fix this issue (we get a 'load' event for the second load). I've not been able to track down the relevant code that's actually making that happen yet, but I'll keep digging.

Comment 6

[Jonathan Watt [:jwatt]]

What happens seems to be this:

For the first load of a document we do a full load under the call stack:

  nsURILoader::OpenURI
  nsDocShell::DoChannelLoad
  nsDocShell::DoURILoad
  nsDocShell::InternalLoad
  nsDocShell::LoadURI
  nsDocShell::LoadURIWithOptions
  nsDocShell::LoadURI

During the second load where the URL differs only by the hash we would normally return early in the `if (doShortCircuitedLoad)` block in nsDocShell::InternalLoad and never call DoURILoad:

https://dxr.mozilla.org/mozilla-central/rev/fc194660762d1b92e1679d860a8bf41116d0f54f/docshell/base/nsDocShell.cpp#10483

However, the `if (doShortCircuitedLoad)` block is itself inside the following `if` statement:

  if (aLoadType == LOAD_NORMAL ||
      aLoadType == LOAD_STOP_CONTENT ||
      LOAD_TYPE_HAS_FLAGS(aLoadType, LOAD_FLAGS_REPLACE_HISTORY) ||
      aLoadType == LOAD_HISTORY ||
      aLoadType == LOAD_LINK) {

In the case that we use LOAD_FLAGS_BYPASS_CACHE, say, the value for aLoadType is LOAD_NORMAL_BYPASS_CACHE, since that's what the ConvertDocShellLoadInfoToLoadType call (in the caller nsDocShell::LoadURI) returns when passed nsIDocShellLoadInfo::loadNormalBypassCache here:

https://dxr.mozilla.org/mozilla-central/rev/fc194660762d1b92e1679d860a8bf41116d0f54f/docshell/base/nsDocShell.cpp#1335

Similarly, if we use LOAD_FLAGS_IS_REFRESH then aLoadType is LOAD_REFRESH.

Obviously neither LOAD_NORMAL_BYPASS_CACHE nor LOAD_REFRESH match the condition to enter the `if` block given above, so we don't enter the `if (doShortCircuitedLoad)` block.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8928324 [details] [diff] [review]
patch

Ah, I missed the big check around that whole block.  Thank you for hunting that down.

Could you add a comment in the docshell code that there are consumers like this one depending on that check, so if it changes they might need to be updated?

r=me with that and the reftest manifest changes to remove the skip-if bits I mentioned above.

Comment 8

[Jonathan Watt [:jwatt]]

Do you have an opinion on whether we should use LOAD_FLAGS_IS_REFRESH or LOAD_FLAGS_BYPASS_CACHE? If it wasn't for the (possibly obsolete) "we should probably deprecate and remove it" comment I mentioned in comment 0 I would actually lean towards the former given that in principle it would do less work/be more accurately what we actually want.

Comment 9

[Boris Zbarsky [:bzbarsky]]

LOAD_FLAGS_IS_REFRESH is specifically for <meta http-equiv="Refresh"> bits.  And it's pretty much unused and has XXX comments about removing it.

So I wold much prefer LOAD_FLAGS_BYPASS_CACHE.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f5ffd87cd5
part 0 - Fix race in layout/reftests/css-ui/caret-color-01-ref.html. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a85c4bd47e4
part 1 - Make the reftest harness support loading of consecutive URIs differing only by hash. r=dbaron,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b174094be3
part 2 - Remove skip-if(styloVsGecko) for various reftests. r=bz

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Pulsebot from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9a85c4bd47e4
> part 1 - Make the reftest harness support loading of consecutive URIs
> differing only by hash. r=dbaron,bz

In the end I did restrict the use of LOAD_FLAGS_BYPASS_CACHE to when the URIs are the same except for the hash since there are only five tests that need it, and using it for everything would mean that we'd unnecessarily force reload common images, fonts, etc. for everything.

Comment 12

[Jonathan Watt [:jwatt]]

Darn, forgot to fold in a fixup.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd6eb876d3d
fixup part 2 - Guard against gCurrentURL being null. r=me

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for reftest failures, e.g. in box-decoration-break-with-outset-box-shadow-1.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/814d5927ffb8a5398809dcafd1e218a7a325ba5e

Followup push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cdd6eb876d3d55218324868eae1f0eb1a9a3c0df&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry
Failure log example box-decoration-break-with-outset-box-shadow-1.html: https://treeherder.mozilla.org/logviewer.html#?job_id=146794014&repo=mozilla-inbound

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2374657677b7
part 1 - Fix race in layout/reftests/css-ui/caret-color-01-ref.html. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf26cbb99b6a
part 2 - Make the reftest harness support loading of consecutive URIs differing only by hash. r=dbaron,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8edbddf6b5
part 3 - Tweak the fuzz on four reftests. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/66d4e6df73d2
part 4 - Remove skip-if(styloVsGecko) for various reftests. r=bz

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2374657677b7
https://hg.mozilla.org/mozilla-central/rev/bf26cbb99b6a
https://hg.mozilla.org/mozilla-central/rev/5f8edbddf6b5
https://hg.mozilla.org/mozilla-central/rev/66d4e6df73d2

Comment 17

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/projects/oak/rev/a9f5ffd87cd50434a80861d311ffd21d3b9989d4
Bug 1417251, part 0 - Fix race in layout/reftests/css-ui/caret-color-01-ref.html. r=me

https://hg.mozilla.org/projects/oak/rev/9a85c4bd47e4122885bb793fd2e91dc9079ba0f6
Bug 1417251, part 1 - Make the reftest harness support loading of consecutive URIs differing only by hash. r=dbaron,bz

https://hg.mozilla.org/projects/oak/rev/c2b174094be367b3057b3f5b92c1be0cf677e9f1
Bug 1417251, part 2 - Remove skip-if(styloVsGecko) for various reftests. r=bz

https://hg.mozilla.org/projects/oak/rev/2374657677b7e599ae60a6c994524e6d2c8b44d0
Bug 1417251, part 1 - Fix race in layout/reftests/css-ui/caret-color-01-ref.html. r=me

https://hg.mozilla.org/projects/oak/rev/bf26cbb99b6acfc4c690b8edb1ec89da89232c67
Bug 1417251, part 2 - Make the reftest harness support loading of consecutive URIs differing only by hash. r=dbaron,bz

https://hg.mozilla.org/projects/oak/rev/5f8edbddf6b51e1af440c3e5de882d5f7c7e102d
Bug 1417251, part 3 - Tweak the fuzz on four reftests. r=me

https://hg.mozilla.org/projects/oak/rev/66d4e6df73d288b7b93d691f2c26fe285d6b6d32
Bug 1417251, part 4 - Remove skip-if(styloVsGecko) for various reftests. r=bz

Comment 18

[Narcis Beleuzu [:NarcisB]]

Backed out for reftest failures in abs-pos-non-replaced-vrl* on Windows and in box-sizing-replaced* on Android. 

https://hg.mozilla.org/mozilla-central/rev/a461fe03fdb07218b7f50e92c59dde64b8f8a5b0
abs-pos-non-replaced-vrl* log --- https://treeherder.mozilla.org/logviewer.html#?job_id=150232672&repo=mozilla-inbound&lineNumber=7097
box-sizing-replaced* log ---https://treeherder.mozilla.org/logviewer.html#?job_id=150224237&repo=mozilla-inbound&lineNumber=2991