Closed Bug 1581859 Opened 2 years ago Closed 2 months ago

Rewrite WebNavigationContent.js in Fission-compatible C++

Categories

(WebExtensions :: General, task, P2)

task

Tracking

(Fission Milestone:M7a, firefox89 fixed)

RESOLVED FIXED
89 Branch
Fission Milestone M7a
Tracking Status
firefox89 --- fixed

People

(Reporter: zombie, Assigned: kmag)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: fission-webext)

Attachments

(11 files, 6 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
No description provided.
Priority: -- → P2

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Fission Milestone: --- → ?

M6

Fission Milestone: ? → M6

This is the kind of thing that should really be rewritten in C++. In any case, I'm not really sure it makes sense as a JSWindowActor, since it's meant to span navigations, and JSWindowActors are tied to a specific inner window.

Fission Milestone: M6 → ?

Tracking for Fission Nightly (M6)

Fission Milestone: ? → M6

Please triage this into M6b or M6b if its Nightly blocking or M7 is it is Beta blocking.

Flags: needinfo?(tomica)

I'm not actually sure what to do with this bug. As Kris mentions above, this isn't the best match for a JSWindowActor, but I'm probably not experienced enough with our internals to do it in C++.

Kris, could you perhaps describe in some detail what exactly this would involve? So that I, or maybe Kashav/Anny could have a go at it?

Otherwise, I think I this could easily be a JSProcessActor, which will be much better, but unsure if good enough for memory reasons.

Flags: needinfo?(kmaglione+bmo)

This bug doesn't seem to need to block our Nightly experiment, so I'm moving it from our old M6 milestone to M6c (so we revisit this bug before our Beta experiment).

Fission Milestone: M6 → M6c

It could be made into a process actor with a bit of effort, but I think converting it to C++ is really the correct solution.

Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → kmaglione+bmo
Status: NEW → ASSIGNED
Summary: Rewrite WebNavigationContent.js as a JSWindowActor → Rewrite WebNavigationContent.js in Fission-compatible C++

Doesn't need to block Fission Nightly experiments.

kmag says it should block Fission Beta because it affects WebExtensions. WebNavigation API currently doesn't work on out-of-process frames.

Severity: normal → N/A
Fission Milestone: M6c → M7
Flags: needinfo?(tomica)
Whiteboard: fission-webext

The need for this has come up fairly often recently, and it greatly simplifies
the JS-to-C++ conversion I'm doing in this bug.

In the future, I'll probably make this a non-refcounted class and add a
subclass with templatized native value types, but the current version is close
to the bare minimum I need for this bug, and I don't want to delay it.

Ideally this would use a JS WeakSet for its implementation, but there's
currently no public API, and I'd rather not take the time to add one just now.
A WeakMap works well enough for the use cases of this bug, though.

In the future, I'll probably change this (along with WeakMap) to not be ref
counted, but I don't want to delay these patches for the sake of that.

The extension framework keeps track of the previous URI for outer windows
using WeakMaps so that it can compare against them when it receives
onLocationChange events. That mostly works, but it's not especially efficient.
Storing it directly in the DocShell is much cleaner, and makes it available to
other consumers as well.

Ideally we'd probably want a version of this available in the parent process
which supports cross-process navigations, but the extension framework only
cares about it for same-document navigations, so it's fine as-is for those
purposes.

There currently isn't a typesafe way to get a property with a given interface
from a property bag from C++. This patch adds one, following the pattern of
similar helpers, like do_GetInterface.

This is the start of an actor which will be automatically instantiated in the
parent and each content process which can be used to route most process-level
IPC traffic needed by the extensions framework. It should allow the extensions
framework to keep its IPC glue close to the code that uses it, and simplify
matters for child-side code which needs to run in both parent and content
processes.

This is a skeleton class which will be instantiated at startup in each
process, and eventually track the same events that the deprecated
WebNavigationContent.js frame script currently tracks.

Actual implementation is added in follow-up patches.

This mirrors the current form submit tracker in WebNavigationContent.js, and
stores a WeakSet entry for a WindowGlobal whenever it receives a DOMFormSubmit
event.

These entries are consumed by code added in follow-up patches.

This ports the WebProgressListener logic from WebNavigationContent.js to the
C++ implementation, and adds IPC messages to send them to the parent process.
Linkage between the parent IPDL endpoints and the listeners in
WebNavigation.jsm is added in a subsequent patch.

This ports the last remaining piece of WebNavigationContent.js, the
DOMContentLoaded event listener, to C++ and adds IPC messages to notify the
parent process of the events. Linkage between the parent IPDL endpoints and
the parent WebNavigation.jsm listeners is added in a subsequent patch.

This migrates WebNavigation.jsm to use messages from PExtensionsParent
generated by the native WebNavigationContent class, and removes the now-unused
WebNavigationContent.js frame script.

Blocks: 1685424

The naming scheme references the hashchange event. This is required by the
extensions framework to call the appropriate navigation listeners when only
the reference fragment is updated by a navigation.

The extension framework needs to set specific flags on navigations triggered
by meta refresh headers. Adding this to the load type of all navigations
triggered by meta refreshes allows it to do this without tracking docshells on
which refreshes are attempted so that it can make inferences.

The naming scheme references the hashchange event. This is required by the
extensions framework to call the appropriate navigation listeners when only
the reference fragment is updated by a navigation.

The extension framework needs to set specific flags on navigations triggered
by meta refresh headers. Adding this to the load type of all navigations
triggered by meta refreshes allows it to do this without tracking docshells on
which refreshes are attempted so that it can make inferences.

Attachment #9209623 - Attachment is obsolete: true
Attachment #9209622 - Attachment is obsolete: true
Attachment #9199614 - Attachment is obsolete: true
Attachment #9199615 - Attachment is obsolete: true
Attachment #9199616 - Attachment is obsolete: true
Attachment #9199621 - Attachment is obsolete: true
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/55a4c2fc561d
Part 2a - Add LOCATION_CHANGE_HASHCHANGE onLocationChange flag. r=nika
https://hg.mozilla.org/integration/autoland/rev/0eb043b5c904
Part 2b - Add LOAD_FLAGS_IS_REFRESH to all navigations triggered by meta refreshes. r=nika
https://hg.mozilla.org/integration/autoland/rev/98ba6b96c900
Part 3a - Add do_GetProperty helper for nsIPropertyBag2. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/92359266d420
Part 3b - Update existing GetPropertyAsInterface callers to use typesafe do_GetProperty instead. r=mccr8,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/d2b8ee78745d
Part 4a - Add skeleton PExtensions actor. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/9c7d0015c616
Part 4b - Add skeleton WebNavigationContent class. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/f7c39feb502e
Part 4d - Add web progress listeners to WebNavigationContent. r=nika
https://hg.mozilla.org/integration/autoland/rev/181c3f29f222
Part 4e - Add DOMContentLoaded listener to WebNavigationContent. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/2a3a77ae1eae
Part 4f - Migrate to the native WebNavigation implementation. r=zombie
Fission Milestone: M7 → M7a

The IPDL source added in this stack changes the unified protocol chunking and
triggers problems caused by the missing include.

Flags: needinfo?(kmaglione+bmo)
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2b68d2eee18e
Part 2a - Add LOCATION_CHANGE_HASHCHANGE onLocationChange flag. r=nika
https://hg.mozilla.org/integration/autoland/rev/6fd5aa7895e3
Part 2b - Add LOAD_FLAGS_IS_REFRESH to all navigations triggered by meta refreshes. r=nika
https://hg.mozilla.org/integration/autoland/rev/816643de7694
Part 3a - Add do_GetProperty helper for nsIPropertyBag2. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/16831d45d0ed
Part 3b - Update existing GetPropertyAsInterface callers to use typesafe do_GetProperty instead. r=mccr8,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/22db36f7d16d
Part 4a - Add skeleton PExtensions actor. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/eeff6f501cfc
Part 4b - Add skeleton WebNavigationContent class. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/4287eebc2c77
Part 4d - Add web progress listeners to WebNavigationContent. r=nika
https://hg.mozilla.org/integration/autoland/rev/e23389fc98b5
Part 4e - Add DOMContentLoaded listener to WebNavigationContent. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/07901e457839
Part 4f - Migrate to the native WebNavigation implementation. r=zombie
https://hg.mozilla.org/integration/autoland/rev/6020ec7d7f32
Part 5 - Add js/Value.h include to PromiseNativeHandler.h. r=nika
Backout by btara@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/2434210a7824
Backed out 10 changesets for causing hazard failures on ExtensionsParent.cpp.
Flags: needinfo?(kmaglione+bmo)
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9796577af27a
Part 2a - Add LOCATION_CHANGE_HASHCHANGE onLocationChange flag. r=nika
https://hg.mozilla.org/integration/autoland/rev/e1d7851295fd
Part 2b - Add LOAD_FLAGS_IS_REFRESH to all navigations triggered by meta refreshes. r=nika
https://hg.mozilla.org/integration/autoland/rev/826c62b783c0
Part 3a - Add do_GetProperty helper for nsIPropertyBag2. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/6003469dc449
Part 3b - Update existing GetPropertyAsInterface callers to use typesafe do_GetProperty instead. r=mccr8,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/b4c5ace21b9e
Part 4a - Add skeleton PExtensions actor. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/4199963fe477
Part 4b - Add skeleton WebNavigationContent class. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/ee611f0839ca
Part 4d - Add web progress listeners to WebNavigationContent. r=nika
https://hg.mozilla.org/integration/autoland/rev/9ceaad6dab5b
Part 4e - Add DOMContentLoaded listener to WebNavigationContent. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/0a33cb185fb3
Part 4f - Migrate to the native WebNavigation implementation. r=zombie
https://hg.mozilla.org/integration/autoland/rev/93fe6801a5e2
Part 5 - Add js/Value.h include to PromiseNativeHandler.h. r=nika

Backed out for hazard failures

backout: https://hg.mozilla.org/integration/autoland/rev/3abbeff037f6755b34c242b19b0b363e9db7a3fb

push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=93fe6801a5e23133610f92a3796cb32421499587&group_state=expanded

failure log: https://treeherder.mozilla.org/logviewer?job_id=334240776&repo=autoland&lineNumber=18998 (firstly seen on a subsequent push)

[task 2021-03-24T05:36:30.192Z] TinderboxPrint: rooting hazards<br/>1
[task 2021-03-24T05:36:30.192Z] TinderboxPrint: (unsafe references to unrooted GC pointers)<br/>224
[task 2021-03-24T05:36:30.192Z] TinderboxPrint: (unnecessary roots)<br/>1217
[task 2021-03-24T05:36:30.192Z] TinderboxPrint: missing expected hazards<br/>0
[task 2021-03-24T05:36:30.192Z] TinderboxPrint: heap write hazards<br/>0
[task 2021-03-24T05:36:30.193Z] TEST-UNEXPECTED-FAIL | hazards | unrooted '<returnvalue>' of type 'JS::Value' live across GC call at toolkit/components/extensions/ExtensionsParent.cpp:55
[task 2021-03-24T05:36:30.193Z] TEST-UNEXPECTED-FAIL | hazards | 1 rooting hazards detected
[task 2021-03-24T05:36:30.193Z] TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit "Inspect Task" link for hazard details
[task 2021-03-24T05:36:30.194Z] + grab_artifacts
[task 2021-03-24T05:36:30.194Z] + local artifacts
[task 2021-03-24T05:36:30.194Z] + artifacts=/builds/worker/artifacts
[task 2021-03-24T05:36:30.194Z] + '[' -d /builds/worker/workspace/haz-browser ']'
[task 2021-03-24T05:36:30.194Z] + cd /builds/worker/workspace/haz-browser
[task 2021-03-24T05:36:30.194Z] + ls -lah
[task 2021-03-24T05:36:30.199Z] total 7.2G

Flags: needinfo?(kmaglione+bmo)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 89 Branch → ---
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4817f2a9474a
Part 2a - Add LOCATION_CHANGE_HASHCHANGE onLocationChange flag. r=nika
https://hg.mozilla.org/integration/autoland/rev/f7b615f0996e
Part 2b - Add LOAD_FLAGS_IS_REFRESH to all navigations triggered by meta refreshes. r=nika
https://hg.mozilla.org/integration/autoland/rev/e260ff83e079
Part 3a - Add do_GetProperty helper for nsIPropertyBag2. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/48a00fa9459e
Part 3b - Update existing GetPropertyAsInterface callers to use typesafe do_GetProperty instead. r=mccr8,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/2d4a14c67ba4
Part 4a - Add skeleton PExtensions actor. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/1cefc071f729
Part 4b - Add skeleton WebNavigationContent class. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/0ac1d8c7421d
Part 4d - Add web progress listeners to WebNavigationContent. r=nika
https://hg.mozilla.org/integration/autoland/rev/cb495001c38f
Part 4e - Add DOMContentLoaded listener to WebNavigationContent. r=zombie,nika
https://hg.mozilla.org/integration/autoland/rev/7376b07ffe66
Part 4f - Migrate to the native WebNavigation implementation. r=zombie
https://hg.mozilla.org/integration/autoland/rev/fd71757fa790
Part 5 - Add js/Value.h include to PromiseNativeHandler.h. r=nika
Flags: needinfo?(kmaglione+bmo)
Duplicate of this bug: 1555455
You need to log in before you can comment on or make changes to this bug.