Closed Bug 1593226 Opened 5 years ago Closed 4 years ago

Runtime.executionContextCreated and Runtime.executionContextDestroyed are not emitted for frames

Categories

(Remote Protocol :: CDP, defect, P1)

defect

Tracking

(firefox70 wontfix, firefox71 wontfix, firefox72 wontfix, firefox78 fixed)

RESOLVED FIXED
Firefox 78
Tracking Status
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox78 --- fixed

People

(Reporter: marusak.matej, Assigned: whimboo)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [puppeteer-beta-mvp])

Attachments

(6 files, 6 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

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.

Flags: needinfo?(marusak.matej)

Indeed, last release without works as expected and first release with has this broken behavior.

Flags: needinfo?(marusak.matej)

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Regressed by: 1564360
Whiteboard: [puppeteer-alpha]
Keywords: regression

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 :)

Flags: needinfo?(hskupin)

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.

Flags: needinfo?(hskupin)

Henrik, next week is RC week, we build our last beta this Thursday :)

Flags: needinfo?(hskupin)

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.

Flags: needinfo?(hskupin)

(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.

Oh, thanks! Then I will have a look as soon as I can.

Blocks: 1591979
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
Blocks: 1549529
Summary: executionContextCreated no longer emitted for all frames → Runtime.executionContextCreated no longer emitted for all frames

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
Depends on: 1549512
Depends on: 1599413
No longer depends on: 1549512

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.

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Depends on: 1565162
Priority: P1 → P3
Whiteboard: [puppeteer-alpha]

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: nobody → hskupin
Blocks: 1599413
Status: NEW → ASSIGNED
No longer depends on: 1599413
Priority: P3 → P1
Whiteboard: [puppeteer-beta-reserve]
No longer blocks: 1599413
Depends on: 1599413

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.

Whiteboard: [puppeteer-beta-reserve] → [puppeteer-beta-mvp]
Blocks: 1603223
Blocks: 1623482
Depends on: 1634029

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.

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

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

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

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 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.

Attachment #9144295 - Attachment is obsolete: true

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.

Attachment #9144296 - Attachment is obsolete: true

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.

Attachment #9144297 - Attachment is obsolete: true

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.

Attachment #9144298 - Attachment is obsolete: true

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.

Attachment #9144299 - Attachment is obsolete: true
Depends on: 1635568
No longer depends on: 1635568

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.

Blocks: 1599413
No longer depends on: 1565162, 1599413
Blocks: 1587452

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.

Also noticed that we always emit the Runtime.executionContextsCleared event even with the Runtime domain disabled.

Blocks: 1549532

Actually the Runtime.executionContextDestroyed event is also affected. Reflecting that in the summary.

Blocks: 1549530
No longer blocks: 1549532
Summary: Runtime.executionContextCreated no longer emitted for all frames → Runtime.executionContextCreated and Runtime.executionContextDestroyed are not emitted for frames
Blocks: 1636384

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.

Attachment #9147222 - Attachment description: Bug 1593226 - [remote] Emit Runtime.executionContextCreated events also for frames. → Bug 1593226 - [remote] Emit Runtime.executionContextCreated and Runtime.executionContextDestroyed events also for frames.
Blocks: 1603984
Blocks: 1637640
Attachment #9148119 - Attachment is obsolete: true

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.

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41a0320ca962
[remote] Create a single context observer per content session. r=remote-protocol-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/24bf7808c6ae
[remote] Return execution context instead of only the id in "enableRuntime" helper. r=remote-protocol-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/ac593bf81996
[remote] Only emit Runtime.executionContextsCleared events when Runtime is enabled. r=remote-protocol-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/996b843543fb
[remote] Emit Runtime.executionContextCreated and Runtime.executionContextDestroyed events also for frames. r=remote-protocol-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/22bfb935c8fd
[remote] Add support for frameId argument to Page.createIsolatedWorld. r=remote-protocol-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/cd96d8a081cc
[remote] Use frame's window for dynamic isolated world creation on new document. r=remote-protocol-reviewers,maja_zf
Regressions: 1638196
Component: CDP: Runtime → CDP
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: