1581859 - Rewrite WebNavigationContent.js in Fission-compatible C++

Status: RESOLVED FIXED

(WebExtensions :: General, task, P2)

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

M6

Comment 3

[Kris Maglione [:kmag]]

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.

Comment 4

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 5

[Neha Kochar [:neha]]

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

Comment 6

[Tomislav Jovanovic :zombie]

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.

Comment 7

[Chris Peterson [:cpeterson]]

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

Comment 8

[Kris Maglione [:kmag]]

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.

Comment 9

[Chris Peterson [:cpeterson]]

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.

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 1a - Add Has and Delete methods to the public WeakMap API. r=tcampbell

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 1b - Add DOM WeakMap wrapper. r=mccr8

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.

Comment 12

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 1c - Add DOM WeakSet wrapper. r=mccr8

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.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 2 - Add nsIWebNavigation::previousURI getter. r=nika

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.

Comment 14

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 3a - Add do_GetProperty helper for nsIPropertyBag2. r=mccr8

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.

Comment 15

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 3b - Update existing GetPropertyAsInterface callers to use typesafe do_GetProperty instead. r=mccr8

Comment 16

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 4a - Add skeleton PExtensions actor. r=nika,zombie

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.

Comment 17

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 4b - Add skeleton WebNavigationContent class. r=zombie,nika

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.

Comment 18

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 4c - Add DOMFormSubmit event tracker. r=zombie,nika

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.

Comment 19

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 4d - Add web progress listeners to WebNavigationContent. r=zombie,nika

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.

Comment 20

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 4e - Add DOMContentLoaded listener to WebNavigationContent. r=zombie,nika

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.

Comment 21

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 4f - Migrate to the native WebNavigation implementation. r=zombie

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

Comment 22

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 2a - Add LOCATION_CHANGE_HASHCHANGE onLocationChange flag. r=nika

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.

Comment 23

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 2b - Add LOAD_FLAGS_IS_REFRESH to all navigations triggered by meta refreshes. r=nika

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.

Comment 24

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 2a - Add LOCATION_CHANGE_HASHCHANGE onLocationChange flag. r=nika

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.

Comment 25

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 2b - Add LOAD_FLAGS_IS_REFRESH to all navigations triggered by meta refreshes. r=nika

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.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages jsapi.h

Backout link: https://hg.mozilla.org/integration/autoland/rev/616c7f5da6dbf2cdded23e0afb2f5cce537e6129
Log link: https://treeherder.mozilla.org/logviewer?job_id=333594576&repo=autoland&lineNumber=44338

Comment 28

[Kris Maglione [:kmag]]

Attachment: Bug 1581859: Part 5 - Add js/Value.h include to PromiseNativeHandler.h. r=nika

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Cosmin Sabou [:CosminS]]

Backed out for causing hazard failures on ExtensionsParent.cpp.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=hazard&revision=6020ec7d7f32a3bb8d581641286bbdb6d2ad8e30&selectedTaskRun=L3p4WO-VR_y70TUyhEVyKw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=334092361&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/75e750d8ff501b363b214b204d948930db6269e4

Comment 31

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/2434210a7824
Backed out 10 changesets for causing hazard failures on ExtensionsParent.cpp.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
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

Comment 33

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/9796577af27a
https://hg.mozilla.org/mozilla-central/rev/e1d7851295fd
https://hg.mozilla.org/mozilla-central/rev/826c62b783c0
https://hg.mozilla.org/mozilla-central/rev/6003469dc449
https://hg.mozilla.org/mozilla-central/rev/b4c5ace21b9e
https://hg.mozilla.org/mozilla-central/rev/4199963fe477
https://hg.mozilla.org/mozilla-central/rev/ee611f0839ca
https://hg.mozilla.org/mozilla-central/rev/9ceaad6dab5b
https://hg.mozilla.org/mozilla-central/rev/0a33cb185fb3
https://hg.mozilla.org/mozilla-central/rev/93fe6801a5e2

Comment 34

[Natalia Csoregi [:nataliaCs]]

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

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/3abbeff037f6

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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

Comment 37

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/4817f2a9474a
https://hg.mozilla.org/mozilla-central/rev/f7b615f0996e
https://hg.mozilla.org/mozilla-central/rev/e260ff83e079
https://hg.mozilla.org/mozilla-central/rev/48a00fa9459e
https://hg.mozilla.org/mozilla-central/rev/2d4a14c67ba4
https://hg.mozilla.org/mozilla-central/rev/1cefc071f729
https://hg.mozilla.org/mozilla-central/rev/0ac1d8c7421d
https://hg.mozilla.org/mozilla-central/rev/cb495001c38f
https://hg.mozilla.org/mozilla-central/rev/7376b07ffe66
https://hg.mozilla.org/mozilla-central/rev/fd71757fa790