Closed Bug 1669961 Opened 1 year ago Closed 1 year ago

JSWindowActorChild .contentWindow getter returns a WindowProxy even when the inner window isn't current

Categories

(Core :: DOM: Content Processes, defect, P2)

defect

Tracking

()

RESOLVED FIXED
84 Branch
Fission Milestone M7
Tracking Status
firefox84 --- fixed

People

(Reporter: jdescottes, Assigned: kmag)

References

Details

(Keywords: perf-alert, Whiteboard: [marionette-fission-mvp])

Attachments

(1 file)

In a JSWindowActorChild, this.document and this.contentWindow.document can point to different objects.

For instance, in the patch at https://phabricator.services.mozilla.com/D92648#2985445 , when the document has not yet reached the "complete" readyState, the two objects will be different. Example of differences:

  • this.document.readyState is "complete" while this.contentWindow.document.readyState is "loading"
  • this.document.location is null while this.contentWindow.location.href points to the expected URL

Looking at the implementation https://searchfox.org/mozilla-central/rev/f82d5c549f046cb64ce5602bfd894b7ae807c8f8/dom/ipc/jsactor/JSWindowActorChild.cpp#76-109, the document getter is not relying on the contentWindow getter, which explains why the two objects can differ.

Is the difference intentional? If it is, are there guidelines or documentation to know when we should use this.document vs this.contentWindow.document?

Was clarified by a discussion on #Fission

this.document and this.contentWindow.document can be out of sync.
this.contentWindow is a windowproxy which might point to a window from another process.
Under some circumstances, this.document might point to a destroyed document, while this.contentWindow.document will point to the document of another window.

this.document should always be considered as the source of truth and used rather than this.contentWindow.document.

Could be interesting to mention at https://firefox-source-docs.mozilla.org/dom/ipc/jsactors.html#this-contentwindow , but I'm not sure how to phrase the warning properly. Closing for now, feel free to reopen if anyone wants to update the doc.

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → INVALID
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: JSWindowActorChild document and contentWindow.document can point to different objects → JSWindowActorChild .contentWindow getter returns a WindowProxy even when the inner window isn't current
Fission Milestone: --- → ?
Whiteboard: [marionette-fission-mvp]
See Also: → 1670332
Assignee: nobody → kmaglione+bmo
Severity: -- → S3
Type: task → defect
Fission Milestone: ? → M7
Priority: -- → P3
Status: REOPENED → ASSIGNED
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/77c34aa0aaf3
Return null from `.contentWindow` when inner window is inactive. r=nika

Backed out for causing crashtests due to windowUtils not being accessible

backout: https://hg.mozilla.org/integration/autoland/rev/c9af08be84bdbaf03622e7e0f8e4ad747cd74707

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=77c34aa0aaf3b522e2e28e2d2d0ee25a3fa5fc42&searchStr=crashtest&selectedTaskRun=K_Dc6RsaR9awX8u1XiHpfQ.0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319422376&repo=autoland&lineNumber=2274

[task 2020-10-22T19:49:04.674Z] 19:49:04 INFO - REFTEST INFO | SET PREFERENCE pref(browser.send_pings,true)
[task 2020-10-22T19:49:04.674Z] 19:49:04 INFO - REFTEST TEST-LOAD | file:///Users/cltbld/tasks/task_1603395997/build/tests/reftest/tests/docshell/base/crashtests/1257730-1.html | 34 / 3856 (0%)
[task 2020-10-22T19:49:04.680Z] 19:49:04 ERROR - REFTEST ERROR | REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/tasks/task_1603395997/build/tests/reftest/tests/docshell/base/crashtests/1257730-1.html | [CONTENT] flushWindow failed: TypeError: can't access property "windowUtils", win is null
[task 2020-10-22T19:49:04.680Z] 19:49:04 ERROR -
[task 2020-10-22T19:49:04.683Z] 19:49:04 INFO - REFTEST TEST-PASS | docshell/base/crashtests/1257730-1.html | (LOAD ONLY)
[task 2020-10-22T19:49:04.684Z] 19:49:04 INFO - REFTEST TEST-END | docshell/base/crashtests/1257730-1.html

Flags: needinfo?(kmaglione+bmo)
Priority: P3 → P2

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:kmag, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/befecdb77ef7
Return null from `.contentWindow` when inner window is inactive. r=nika
Status: ASSIGNED → RESOLVED
Closed: 1 year ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch

== Change summary for alert #27616 (as of Thu, 12 Nov 2020 16:27:51 GMT) ==

Regressions:

Ratio Suite Test Platform Options Absolute values (old vs new)
2% Base Content JS macosx1014-64-shippable-qr 2,798,629.33 -> 2,841,002.67

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=27616

You need to log in before you can comment on or make changes to this bug.