1766305 - Make browser_test_shentry_wireframe.js work with parent controlled loads

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Andrew McCreight [:mccr8]]

This test fails with browser.tabs.documentchannel.parent-controlled set to true. The feature seems to include some interaction with SHEntry stuff, so it is possible that this is an actual issue with the feature, and not just an issue with the test (some tests have to be fixed to wait for different events when run with parent controlled loads).

Running with
./mach mochitest --headless --setpref browser.tabs.documentchannel.parent-controlled=true browser_test_shentry_wireframe.js
results in these failures:

docshell/test/navigation/browser_test_shentry_wireframe.js
  FAIL A wireframe was captured for the first entry. - null == true - JS frame :: chrome://mochitests/content/browser/docshell/test/navigation/browser_test_shentry_wireframe.js :: <TOP_LEVEL> :: line 34
Stack trace:
chrome://mochitests/content/browser/docshell/test/navigation/browser_test_shentry_wireframe.js:null:34
  UNEXPECTED-PASS No wireframe for the loaded entry. - 
Stack trace:
chrome://mochitests/content/browser/docshell/test/navigation/browser_test_shentry_wireframe.js:null:52
  FAIL A wireframe was captured for the first entry. - null == true - JS frame :: chrome://mochitests/content/browser/docshell/test/navigation/browser_test_shentry_wireframe.js :: <TOP_LEVEL> :: line 63
Stack trace:
chrome://mochitests/content/browser/docshell/test/navigation/browser_test_shentry_wireframe.js:null:63
  FAIL A wireframe was captured for the first entry. - null == true - JS frame :: chrome://mochitests/content/browser/docshell/test/navigation/browser_test_shentry_wireframe.js :: <TOP_LEVEL> :: line 79
Stack trace:
chrome://mochitests/content/browser/docshell/test/navigation/browser_test_shentry_wireframe.js:null:79

Comment 1

[Andrew McCreight [:mccr8]]

It looks like PersistLayoutHistoryState() (which is where CollectWireframe is called) is called from nsDocShell::DoURILoad when the pref is false, but not when it is true.

Comment 2

[Andrew McCreight [:mccr8]]

Saving wireframes happens in the same method as saving scroll positions, so maybe this is related to bug 1743636. I'll have to read over Anny's comments there.

Comment 3

[Andrew McCreight [:mccr8]]

Yeah, it sounds like the issue is very similar to what Anny described in bug 1743636 comment 4: the test loads one page from the parent, then a second from the parent, and then somehow it doesn't save the layout history stuff, which includes both the scroll position (the other test) and the wireframe.

Comment 4

[Andrew McCreight [:mccr8]]

I managed to get at least a prototype of something that at least passes 1 test that wasn't passing before. At some point, somebody pointed me at bug 1766131, where Peter added SynchronizeLayoutHistoryState(). This saves the current session history entry, requests some layout state from the child, then saves that layout data into the entry. This avoids the issue of the wireframe data being sent up from the child not having an active entry.

After far too much time spent wrestling with C++ compilation errors, I managed to a wireframe to the data that is returned. Once you have it along with the prior active entry, it is easy to save it. Then, I added a call to SynchronizeLayoutHistoryState() into CanonicalBrowsingContext::ReplacedBy(), which is where the active entry is changed in this case.

Comment 5

[Andrew McCreight [:mccr8]]

That also seems to fix browser_scrollPositions.js, so that's nice.

Comment 6

[Andrew McCreight [:mccr8]]

Here's my WIP patch for this issue: https://phabricator.services.mozilla.com/differential/diff/578590/

While it does make part of the test pass, it doesn't fix everything.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Bug 1766305, part 2 - Return a wireframe with GetLayoutHistoryState.

Comment 8

[Andrew McCreight [:mccr8]]

The updated version of the patch passes all subtests, with and without parent controlled navigation. It even passes the two "todo" tests. I still need to go through and see if the existing CollectWireframe is still necessary, as it feels like it might be redundant.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Bug 1766305, part 1 - Give browser_test_shentry_wireframe.js subtests different descriptions.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d966ca8f0bd
part 1 - Give browser_test_shentry_wireframe.js subtests different descriptions. r=mconley
https://hg.mozilla.org/integration/autoland/rev/61c86361ac7c
part 2 - Return a wireframe with GetLayoutHistoryState. r=smaug,mconley

Comment 12

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/8d966ca8f0bd
https://hg.mozilla.org/mozilla-central/rev/61c86361ac7c