Runtime.executionContextCreated and Runtime.executionContextDestroyed are not emitted for frames
Categories
(Remote Protocol :: CDP, defect, P1)
Tracking
(firefox70 wontfix, firefox71 wontfix, firefox72 wontfix, firefox78 fixed)
People
(Reporter: marusak.matej, Assigned: whimboo)
References
(Blocks 5 open bugs, Regression)
Details
(Keywords: regression, Whiteboard: [puppeteer-beta-mvp])
Attachments
(6 files, 6 obsolete files)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0
Steps to reproduce:
I am implementing testing with CDP for Firefox in Cockpit project [1]. I was using Firefox 69 (firefox-69.0.1-3.fc31.x86_64 Fedora 30/31) and when page with iframe was loaded I was able to catch Runtime.ExecutionContextCreated event twice (one for the default/top most page and one for the iframe). With Firefox 70 (firefox-70.0-1.fc31.x86_64.rpm Fedora 31) I get only one such event (for the default frame).
I also tested it with Firefox nightly and same unwanted behavior.
I don't have any standalone reproducer, but it should be fairly simple to reproduce - just have page with <iframe> in it. I am happy to provide any more needed information if requested, since this is actual blocker for us.
[1] https://github.com/cockpit-project/cockpit/pull/12971
Actual results:
I get only one executionContextCreated
instead of expected 2 (one for the page itself and one for iframe). Maybe I should note that we need this to work with main page and a few (usually 2-4, but up to 10) iframes at once.
Expected results:
I expect to get n+1 events, n being number of iframes in the page.
Assignee | ||
Comment 1•5 years ago
|
||
With bug 1564360 some changes have been landed for Firefox 70 which ignore iframes for Page.frameNavigated
. Maybe that was causing it.
Marusak, can you please go to the following URL and test the builds from before and after this change?
https://hg.mozilla.org/mozilla-central/rev/4b05fdad2e13
You can find those under first release with
and last release without
. Thanks.
Reporter | ||
Comment 2•5 years ago
|
||
Indeed, last release without
works as expected and first release with
has this broken behavior.
Assignee | ||
Comment 3•5 years ago
|
||
Thank you for the feedback. In such a case we should make sure to have the regression fixed for the alpha release of our Puppeteer support. Also because the Gutenberg tests make use of iframes.
Assignee | ||
Updated•5 years ago
|
Comment 4•5 years ago
|
||
Henrik, I am guessing that you are no planning a late uplift of a patch in 71 beta but I prefer to ask before marking it as wonfix for 71 :)
Assignee | ||
Comment 5•5 years ago
|
||
Yes, I will still try to get this regression fixed in 71, but have to look at some other blockers first. So probably by next week I can get started.
Comment 6•5 years ago
|
||
Henrik, next week is RC week, we build our last beta this Thursday :)
Assignee | ||
Comment 7•5 years ago
|
||
Given that I won't be around on Wednesday it is very unlikely that the fix will make it into 71. Looks like we have to live with it until 72.
Comment 8•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #7)
Given that I won't be around on Wednesday it is very unlikely that
the fix will make it into 71. Looks like we have to live with it
until 72.
Bear in mind that the remote agent is only built on the Nightly
release channel.
Assignee | ||
Comment 9•5 years ago
|
||
Oh, thanks! Then I will have a look as soon as I can.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 10•5 years ago
|
||
Just as a note... Runtime.executionContextCreated
should also be raised multiple times if a page without frames is loaded. Reason is that Puppeteer creates a specific utility world for each frame. This is the output from Chrome when loading a page without frames:
puppeteer:protocol ◀ RECV {"method":"Runtime.executionContextCreated","params":{"context":{"id":3,"origin":"://","name":"","auxData":{"isDefault":true,"type":"default","frameId":"2DDAA7D3C54C543B6317572523DC8309"}}},"sessionId":"FFB942A223718B95690A99BCC5F40E46"} +0ms
puppeteer:protocol ◀ RECV {"method":"Runtime.executionContextCreated","params":{"context":{"id":4,"origin":"://","name":"__puppeteer_utility_world__","auxData":{"isDefault":false,"type":"isolated","frameId":"2DDAA7D3C54C543B6317572523DC8309"}}},"sessionId":"FFB942A223718B95690A99BCC5F40E46"} +0ms
Also we haven't implemented Page.frameAttached
(bug 1549512) yet, which comes first when navigating to a page which includes frames. Here Chrome's output:
puppeteer:protocol ◀ RECV {"method":"Page.frameAttached","params":{"frameId":"F85E5BB0512721B0713F7F42CF8CF001","parentFrameId":"D5D55B7CD8CA314E10E669514A0F3F2C"},"sessionId":"A548BD3B65BD2C49BA29687A9B3A059F"} +1ms
..
puppeteer:protocol ◀ RECV {"method":"Page.frameNavigated","params":{"frame":{"id":"F85E5BB0512721B0713F7F42CF8CF001","parentId":"D5D55B7CD8CA314E10E669514A0F3F2C","loaderId":"4B10B6AF766F445FAD40C1EED4CD2F5C","name":"","url":"http://localhost/~henrik/iframe1.html","securityOrigin":"http://localhost","mimeType":"text/html"}},"sessionId":"A548BD3B65BD2C49BA29687A9B3A059F"} +0ms
puppeteer:protocol ◀ RECV {"method":"Runtime.executionContextCreated","params":{"context":{"id":5,"origin":"http://localhost","name":"","auxData":{"isDefault":true,"type":"default","frameId":"F85E5BB0512721B0713F7F42CF8CF001"}}},"sessionId":"A548BD3B65BD2C49BA29687A9B3A059F"} +0ms
Assignee | ||
Comment 11•5 years ago
|
||
After more investigation on bug 1599413 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 turns out to be a blocker for the Gutenberg and Puppeteer unit tests or examples.
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
This missing feature actually blocks me with implementing Page.frameAttached
on bug 1599413, and also seems to cause hangs in a lot of Puppeteer unit tests. We should clearly give this a higher priority. As such also switching the dependency order.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 13•5 years ago
|
||
When I started to work on that bug I noticed that we actually instantiate a context observer per domain (right now for Page and Runtime), which means we would register a lot of event listeners and observer notifications. That can slow down the browser and causes extra memory, and not speaking about all the duplicated internal events, which get send around. Instead we should make it a singleton per target.
Updated•5 years ago
|
Assignee | ||
Comment 14•5 years ago
|
||
To better keep track of available observer classes
it's better to have them all in the same folder.
While moving files around the patch also renames
the TabObserver module to TargetObserver, which
would allow us to add target observers for workers
in the future.
Assignee | ||
Comment 15•5 years ago
|
||
The changes align our code to other instances of nsIWindowMediatorListener
usage in-tree, which always rely on the "load" event. Also "interactive"
isn't a ready state a XULWindow can ever be in, it's only used for content
windows.
Depends on D73042
Assignee | ||
Comment 16•5 years ago
|
||
The WindowObserver class is only used by the TabObserver, and as such
can easily be integrated transparently. This also removes the extra
events as being emitted for opening and closing XUL windows.
Depends on D73043
Assignee | ||
Comment 17•5 years ago
|
||
Within CDP there is no tab target, but a page target. The patch
renames our TabTarget class appropriately for an easier understanding
of our code.
Depends on D73044
Assignee | ||
Comment 18•5 years ago
|
||
Within CDP there is no tab session, but a page session. The patch
renames our TabSession class appropriately for an easier understanding
of our code.
Depends on D73045
Comment 19•5 years ago
|
||
Comment on attachment 9144295 [details]
Bug 1593226 - [remote] Re-organize observer modules into a single directory.
Revision D73042 was moved to bug 1634029. Setting attachment 9144295 [details] to obsolete.
Comment 20•5 years ago
|
||
Comment on attachment 9144296 [details]
Bug 1593226 - [remote] Simplify handling of created DOMWindow's.
Revision D73043 was moved to bug 1634029. Setting attachment 9144296 [details] to obsolete.
Comment 21•5 years ago
|
||
Comment on attachment 9144297 [details]
Bug 1593226 - [remote] Integrate WindowObserver into TabObserver.
Revision D73044 was moved to bug 1634029. Setting attachment 9144297 [details] to obsolete.
Comment 22•5 years ago
|
||
Comment on attachment 9144298 [details]
Bug 1593226 - [remote] Adjust naming for page target to align with CDP.
Revision D73045 was moved to bug 1634029. Setting attachment 9144298 [details] to obsolete.
Comment 23•5 years ago
|
||
Comment on attachment 9144299 [details]
Bug 1593226 - [remote] Adjust naming for page sessions to align with CDP.
Revision D73046 was moved to bug 1634029. Setting attachment 9144299 [details] to obsolete.
Assignee | ||
Comment 24•5 years ago
|
||
As it turned out we need a short-term solution here. So we cannot wait on the Fission related work. Also bug 1599413 (Page.frameAttached) should be on the blocker list.
Assignee | ||
Comment 25•5 years ago
|
||
To correctly emit the events for sub frames we also have to create an isolated world. Right now Page.createIsolatedWorld
doesn't support the frameId
, so I will add it while working on this bug.
Assignee | ||
Comment 26•5 years ago
|
||
Also noticed that we always emit the Runtime.executionContextsCleared event even with the Runtime domain disabled.
Assignee | ||
Comment 27•5 years ago
|
||
Actually the Runtime.executionContextDestroyed
event is also affected. Reflecting that in the summary.
Assignee | ||
Comment 28•5 years ago
|
||
Domains would have created their own instance of the context observer,
which results in duplicated event listeners and observer notifications
to be registered.
This is still not ideal for the observer notifications, which should be
registered only once, but still an improvement for now. Bug 1635568 will
finally fix that.
Assignee | ||
Comment 29•5 years ago
|
||
Depends on D74632
Assignee | ||
Comment 30•5 years ago
|
||
Depends on D74633
Assignee | ||
Comment 31•5 years ago
|
||
Depends on D74634
Assignee | ||
Comment 32•5 years ago
|
||
Depends on D74635
Updated•5 years ago
|
Assignee | ||
Comment 33•5 years ago
|
||
Depends on D74636
Assignee | ||
Comment 34•5 years ago
|
||
Depends on D75170
Updated•5 years ago
|
Assignee | ||
Comment 35•5 years ago
|
||
I noticed that we cannot simply go ahead with pushing those patches and having only partial frame support. While we can fix our Puppeteer unit test expectations, it cannot be done for upstream Puppeteer tests, or even consumers. Whenever a frame gets loaded it will fail.
As such I will introduce a temporary new preference with the name remote.frames.enabled
that defaults to false
. Only our browser chrome tests will set it for now, given that we need the events there. Otherwise frame events are not getting sent out, and Firefox continues to behave as now. Once all the frame related bugs are fixed, we can remove this preference.
Comment 36•5 years ago
|
||
Comment 37•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/41a0320ca962
https://hg.mozilla.org/mozilla-central/rev/24bf7808c6ae
https://hg.mozilla.org/mozilla-central/rev/ac593bf81996
https://hg.mozilla.org/mozilla-central/rev/996b843543fb
https://hg.mozilla.org/mozilla-central/rev/22bfb935c8fd
https://hg.mozilla.org/mozilla-central/rev/cd96d8a081cc
Updated•4 years ago
|
Updated•3 years ago
|
Description
•