Closed Bug 1599413 Opened 5 years ago Closed 5 years ago

Implement Page.frameAttached

Categories

(Remote Protocol :: CDP, task, P1)

Product:
Remote Protocol
Component:
CDP
Type:
task

Tracking

(firefox78 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 78
Tracking Flags:
Tracking Status
firefox78 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(
URL
)

Details

(Whiteboard: [puppeteer-beta-mvp])

Attachments

(2 files)

Bug 1599413 - [remote] Add executeSoon to sync helper methods.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1599413 - [remote] Implement Page.frameAttached.
47 bytes, text/x-phabricator-request
Details | Review

Creating the implementation bug for bug 1549512.

To get this done the framescript will have to use the MutationObserver interface to detect changes to the DOM. There is no specific event which gets raised under such a situation.

Given that MutationObservers are expensive to use in terms of performance I had a further look and found that devtools are using the webnavigation-create and webnavigation-destroy observer notifications:

https://searchfox.org/mozilla-central/rev/0678172d5b5c681061b904c776b668489e3355b0/devtools/server/actors/targets/browsing-context.js#628-632

They implemented it in a Fission-compatible way, which we can't do right now because we have no JSWindowActor support (bug 1565162).

During our meeting Andreas also mentioned Content:BrowserChildReady, but I will have to check if that gives us all the necessary data, and if that is fired at the right time.

Note that the implementation for this bug will not be Fission-compatible, but should make it easy at a later time to just switch over.

After more investigation I don't think it make sense right now to work on a fix for this bug as long as we do not make use of the JSWindowActor classes to support Fission. As such we will have to defer the work until bug 1565162 has been fixed (which will happen in Q1 2020), or it still turns out to be a blocker for the Gutenberg and Puppeteer unit tests or examples.

As such moving this bug into the alpha reserve bucket.

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3
Whiteboard: [puppeteer-alpha] → [puppeteer-alpha-reserve]
Priority: P3 → P2
Whiteboard: [puppeteer-alpha-reserve] → [puppeteer-beta-mvp]
No longer depends on: 1609152

I will have a look at this one next. A lot of Puppeteer unit tests depend on it for frame handling.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1

After some investigation yesterday we can actually go ahead without the JSWindowActor implemenation (bug 1565162).

No longer depends on: 1565162
No longer blocks: 1593226
Depends on: 1593226

I thought that I should give an update here. I got all the pieces implemented and here a recent try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49892096de3cc00f526246f84fa01941faa85022&selectedJob=297940598

The failures for puppeteer are all related to bug 1593226 because we do not create new execution contexts inside of frames. As such I would also like to have those fixed first.

Didn't actually meant to swap the dependency order.

Blocks: 1593226
No longer depends on: 1593226

There are needs to delay the execution of code on the main thread
until the next tick of the event loop has happened.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e5dac7407c50 [remote] Add executeSoon to sync helper methods. r=remote-protocol-reviewers,jgraham

(In reply to Pulsebot from comment #9)

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5dac7407c50
[remote] Add executeSoon to sync helper methods.
r=remote-protocol-reviewers,jgraham

Sorry, that was meant for bug 1628281.

Keywords: leave-open
No longer blocks: 1593226
Depends on: 1593226
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/746f1f57bd84 [remote] Implement Page.frameAttached. r=remote-protocol-reviewers,maja_zf

We had a problem with our shared helper loadURL(), which didn't wait for sub-frames to be loaded. As such it returned too early, and caused a race condition. Not sure why this only happens on Linux shippable, but looks like it's the fastest platform. I wasn't even able to reproduce locally under MacOS.

I pushed a new try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc8954d76a57fc99ae55268b56193ce393e6ab60

Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6f223ae549d8 [remote] Implement Page.frameAttached. r=remote-protocol-reviewers,maja_zf

Backed out 2 changesets (bug 1599773, bug 1599413) for browser_frameAttached.js failures.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=800142bdea708f819ee11fadcf08f6cf3170eb32&searchStr=remote&tochange=0f440282ada89ad47b96c81b541e52259bd7eb20&selectedTaskRun=fdhp796-R_iWQlxlyeBWmQ-0

Backout link: https://hg.mozilla.org/integration/autoland/rev/0f440282ada89ad47b96c81b541e52259bd7eb20

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302874776&repo=autoland&lineNumber=4711

...
[task 2020-05-19T09:23:48.098Z] 09:23:48     INFO - Leaving test bound eventWhenNavigatingWithFrames
[task 2020-05-19T09:23:48.099Z] 09:23:48     INFO - Entering test bound eventWhenNavigatingWithNestedFrames
[task 2020-05-19T09:23:48.099Z] 09:23:48     INFO - CDP server started
[task 2020-05-19T09:23:48.100Z] 09:23:48     INFO - Buffered messages logged at 09:23:47
[task 2020-05-19T09:23:48.100Z] 09:23:48     INFO - CDP client instantiated
[task 2020-05-19T09:23:48.101Z] 09:23:48     INFO - Navigate to a page with nested iframes
[task 2020-05-19T09:23:48.102Z] 09:23:48     INFO - TEST-PASS | remote/test/browser/page/browser_frameAttached.js | Expected amount of frames added - 
[task 2020-05-19T09:23:48.102Z] 09:23:48     INFO - TEST-PASS | remote/test/browser/page/browser_frameAttached.js | Received the expected amount of frameAttached events - 
[task 2020-05-19T09:23:48.103Z] 09:23:48     INFO - TEST-PASS | remote/test/browser/page/browser_frameAttached.js | Found expected frame with id 2147483653 - 
[task 2020-05-19T09:23:48.104Z] 09:23:48     INFO - TEST-PASS | remote/test/browser/page/browser_frameAttached.js | Got expected frame id for frameAttached event - 
[task 2020-05-19T09:23:48.104Z] 09:23:48     INFO - TEST-PASS | remote/test/browser/page/browser_frameAttached.js | Got expected parent frame id for frameAttached event - 
[task 2020-05-19T09:23:48.105Z] 09:23:48     INFO - TEST-PASS | remote/test/browser/page/browser_frameAttached.js | Found expected frame with id 2147483654 - 
[task 2020-05-19T09:23:48.105Z] 09:23:48     INFO - TEST-PASS | remote/test/browser/page/browser_frameAttached.js | Got expected frame id for frameAttached event - 
[task 2020-05-19T09:23:48.106Z] 09:23:48     INFO - TEST-PASS | remote/test/browser/page/browser_frameAttached.js | Got expected parent frame id for frameAttached event - 
[task 2020-05-19T09:23:48.107Z] 09:23:48     INFO - CDP client closed
[task 2020-05-19T09:23:48.108Z] 09:23:48     INFO - CDP server stopped
[task 2020-05-19T09:23:48.108Z] 09:23:48     INFO - Leaving test bound eventWhenNavigatingWithNestedFrames
[task 2020-05-19T09:23:48.109Z] 09:23:48     INFO - Entering test bound eventWhenAttachingFrame
[task 2020-05-19T09:23:48.109Z] 09:23:48     INFO - CDP server started
[task 2020-05-19T09:23:48.110Z] 09:23:48     INFO - CDP client instantiated
[task 2020-05-19T09:23:48.110Z] 09:23:48     INFO - TEST-PASS | remote/test/browser/page/browser_frameAttached.js | Expected amount of frames added - 
[task 2020-05-19T09:23:48.111Z] 09:23:48     INFO - Buffered messages finished
[task 2020-05-19T09:23:48.112Z] 09:23:48     INFO - TEST-UNEXPECTED-FAIL | remote/test/browser/page/browser_frameAttached.js | Received the expected amount of frameAttached events - Got 0, expected 1
[task 2020-05-19T09:23:48.112Z] 09:23:48     INFO - Stack trace:
[task 2020-05-19T09:23:48.112Z] 09:23:48     INFO - chrome://mochikit/content/browser-test.js:test_is:1327
[task 2020-05-19T09:23:48.113Z] 09:23:48     INFO - chrome://mochitests/content/browser/remote/test/browser/page/browser_frameAttached.js:runFrameAttachedTest:107
[task 2020-05-19T09:23:48.113Z] 09:23:48     INFO - chrome://mochitests/content/browser/remote/test/browser/page/browser_frameAttached.js:eventWhenAttachingFrame:76
[task 2020-05-19T09:23:48.113Z] 09:23:48     INFO - chrome://mochitests/content/browser/remote/test/browser/head.js:fn:69
[task 2020-05-19T09:23:48.114Z] 09:23:48     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1064
[task 2020-05-19T09:23:48.114Z] 09:23:48     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1104
[task 2020-05-19T09:23:48.114Z] 09:23:48     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:927
[task 2020-05-19T09:23:48.114Z] 09:23:48     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:918
[task 2020-05-19T09:23:48.114Z] 09:23:48     INFO - GECKO(4280) | 1589880227766	RemoteAgent	TRACE	<-(connection {4c88504c-5e50-4f6f-ae65-c42b306ee92d}) {"method":"Target.targetDestroyed","params":{"targetId":"a7e2e8fd-6524-4a30-9fdf-cbf24dfe397c"}}
[task 2020-05-19T09:23:48.114Z] 09:23:48     INFO - CDP client closed
[task 2020-05-19T09:23:48.115Z] 09:23:48     INFO - GECKO(4280) | 1589880227799	RemoteAgent	TRACE	<-(connection {4c88504c-5e50-4f6f-ae65-c42b306ee92d}) {"method":"Target.targetDestroyed","params":{"targetId":"abca3ca7-0def-4be5-a6ca-adbc9766fde1"}}
[task 2020-05-19T09:23:48.116Z] 09:23:48     INFO - GECKO(4280) | 1589880227799	RemoteAgent	TRACE	<-(connection {4c88504c-5e50-4f6f-ae65-c42b306ee92d}) {"method":"Target.targetDestroyed","params":{"targetId":"456997c5-37c2-42ea-a74d-e837968a6891"}}
[task 2020-05-19T09:23:48.116Z] 09:23:48     INFO - CDP server stopped
[task 2020-05-19T09:23:48.116Z] 09:23:48     INFO - Leaving test bound eventWhenAttachingFrame
[task 2020-05-19T09:23:48.114Z] 09:23:48     INFO - CDP client closed
[task 2020-05-19T09:23:48.115Z] 09:23:48     INFO - GECKO(4280) | 1589880227799	RemoteAgent	TRACE	<-(connection {4c88504c-5e50-4f6f-ae65-c42b306ee92d}) {"method":"Target.targetDestroyed","params":{"targetId":"abca3ca7-0def-4be5-a6ca-adbc9766fde1"}}
[task 2020-05-19T09:23:48.116Z] 09:23:48     INFO - GECKO(4280) | 1589880227799	RemoteAgent	TRACE	<-(connection {4c88504c-5e50-4f6f-ae65-c42b306ee92d}) {"method":"Target.targetDestroyed","params":{"targetId":"456997c5-37c2-42ea-a74d-e837968a6891"}}
[task 2020-05-19T09:23:48.116Z] 09:23:48     INFO - CDP server stopped
[task 2020-05-19T09:23:48.116Z] 09:23:48     INFO - Leaving test bound eventWhenAttachingFrame
[task 2020-05-19T09:23:48.117Z] 09:23:48     INFO - GECKO(4280) | MEMORY STAT | vsize 3234MB | residentFast 409MB | heapAllocated 160MB
[task 2020-05-19T09:23:48.118Z] 09:23:48     INFO - TEST-OK | remote/test/browser/page/browser_frameAttached.js | took 3206ms

This is still strange. While the try build shows it all green, on autoland we have a 50% failure rate for shippable builds on Linux. I might have to use an interactive task to figure that out.

The problematic test here is eventWhenAttachingFrame where I took the advice from by using SpecialPowers.spawn instead of Runtime.evaluate to dynamically attach the frame to the DOM. Not sure if that causes the race here. I will update the patch to make use of the history recording as what other tests are doing. Maybe that is more stable.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/62b8adbc0e67 [remote] Implement Page.frameAttached. r=remote-protocol-reviewers,maja_zf

https://hg.mozilla.org/mozilla-central/rev/62b8adbc0e67

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox78: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 78
Component: CDP: Page → CDP
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: