When inspected process swaps on navigation (PSON) inspector is notified asynchronously about the new page creation, only after its load request is committed. The front-end then attaches to the new page and restores states of the agents. By that point the navigation may have made significant progress and the page's scripts may have executed to some point. It means that user agent overrides, inline script breakpoints, main resource load instrumentation etc. will not work. To fix this and bring it on par with same process navigation (same origin) we'd like to try and start creating inspected targets for provisional pages before they start loading. This will enable inspector front-end to intercept the control flow before sending main resource load request and configure all agents at the very early stage. Once the page navigation commits, all agents will be in proper state and no events will be missed for the new page. Another alternative we've discussed on #webkit-inspector was resurrecting inspector state cookie and passing it from old page to the new one transparently to the front-end and hiding all PSON details from the front-end. Such approach is deemed to require more complicated logic on the backend because in addition to the state maintenance it'd have to broadcast commands from the front-end to both current and provisional pages and buffer up responses until navigation commits or gets aborted.
Created attachment 379777 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Created attachment 379967 [details] Patch
Comment on attachment 379967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379967&action=review Generally, the high level idea seems fine. I'd like to have a better grasp of what specifically (and how) you plan on doing with these changes (e.g. what you are implying when you say "For now there are no changes frontend behavior") if possible, as that may help me better fit these changes into my mind :) > Source/WebCore/ChangeLog:11 > + (WebCore::InspectorController::setIsUnderTest): This method maybe closed several times Typo: "maybe closed" => "may be called" > Source/WebInspectorUI/ChangeLog:9 > + no changes frontend bhavior, it will wait for the provisional target to commit Typo: "bhavior" => "behavior" > Source/WebInspectorUI/ChangeLog:25 > + send commands to the provisional targtets yet, it has to ignore all activities Typo: "targtets" => "targets" > Source/WebInspectorUI/ChangeLog:30 > + Better support for provisional targets will be added to frontend in a separate change. Can we have bug URL for this? > Source/WebInspectorUI/ChangeLog:34 > + of tasrgetDestroyed/targetCreated events which matches previous behavior. Typo: "tasrgetDestroyed" => "targetDestroyed" > Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:139 > + return; Style: please add a newline after this return > Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:140 > + InspectorTarget* target = m_targets.get(committedTargetID); NIT: `auto*` > Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:142 > + return; Ditto (139) > Source/JavaScriptCore/inspector/protocol/Target.json:11 > + { "name": "isProvisional", "type": "boolean", "optional": true, "description": "Whether this is a provisional page target." }, NIT: I prefer putting `optional` values at the end of a type's declaration, so it's easier to read what's required vs what's optional > Source/JavaScriptCore/inspector/protocol/Target.json:12 > + { "name": "previousTargetId", "type": "string", "optional": true }, Ditto Also, where is this actually used? I don't see the frontend using this value at all. It uses `oldTargetId` from `Target.event.didCommitProvisionalTarget` instead. Can they be merged? I'd rather have the `oldTargetId` parameter than add another optional value to `Target.TargetInfo`. > Source/WebCore/inspector/InspectorController.cpp:346 > void InspectorController::setIsUnderTest(bool value) NIT: we could inline this in the header now that it's so small/simple > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:311 > // Ignore errors if a worker went away quickly. NIT: please move this comment to be above the `if` so it's clear what it's referring to > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:312 > + WorkerAgent.sendMessageToWorker(this._workerId, message).catch(e => { Style: please use a full/descriptive parameter name, like `error` Style: we always add the `(...)` around the parameters of an arrow function, even if there's only one argument ```js WorkerAgent.sendMessageToWorker(this._workerId, message).catch((error) => { ``` > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:313 > + if (this.target.isDestroyed()) If the target is already destroyed, shouldn't we not even be sending the command to begin with? We should probably add an assertion somewhere `console.assert(!this.target.isDestroyed);`. Or is this to handle the situation where the target is destroyed after the command is dispatched, but before the response is received? > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:315 > + throw e; Rather than re-throw, please use `WI.reportInternalException(error)` instead. We try to avoid using `throw` as much as possible. > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:339 > + TargetAgent.sendMessageToTarget(this._targetId, message).catch(e => { Ditto > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:342 > + if (this.target.isDestroyed()) Ditto > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:344 > + throw e; Ditto > Source/WebInspectorUI/UserInterface/Protocol/Target.js:208 > + isDestroyed() Why not use a getter? ```js get isDestroyed() { return this._isDestroyed; } ``` > Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:32 > + this._provisionalTargetIds = new Set; > + this._swappedTargetIds = new Set; Please move this logic to `WI.TargetManager`. We try to keep our observer classes (which are responsible for handling events received by the frontend) as "clean" of logic as possible, and instead have the various managers and model objects handle the actual logic. > Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:37 > + if (targetInfo.isProvisional) { Please add a compatibility statement. ``` // COMPATIBILITY (iOS 13.0): `Target.TargetInfo.isProvisional` did not exist yet. > Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:44 > + didCommitProvisionalTarget(previousTargetId, targetInfo) Along the lines of 30-31, I'd create a `WI.TargetManager.prototype.didCommitProvisionalTarget` function and move this logic inside of that. I'd strongly prefer keeping the name the same, so it's easier to follow the code through the various layers/transformations. > Source/WebInspectorUI/UserInterface/Test/Test.js:107 > WI.transitionPageTarget = function(target) It seems like we should move these functions to `WI.TargetManager` so they don't get out of sync between 'Main.js' and 'Test.js'. > Source/WebKit/UIProcess/InspectorTargetProxy.cpp:47 > + auto target = create(provisionalPage.page(), targetId, type); NIT: I think for the purposes of code searching, I'd rather you also have the class name here too: ```cpp auto target = InspectorTargetProxy::create(provisionalPage.page(), targetId, type); ``` > Source/WebKit/UIProcess/InspectorTargetProxy.cpp:65 > + } > if (m_page.hasRunningProcess()) Style: missing newline (since the previous `if` has an early return) > Source/WebKit/UIProcess/InspectorTargetProxy.cpp:75 > + } > if (m_page.hasRunningProcess()) Ditto > Source/WebKit/UIProcess/InspectorTargetProxy.cpp:85 > + } > if (m_page.hasRunningProcess()) Ditto > Source/WebKit/UIProcess/InspectorTargetProxy.h:51 > + bool isProvisional() const override; NIT: these could all be `final` instead of `override` (or even omitted, since the class is marked `final`). > Source/WebKit/UIProcess/InspectorTargetProxy.h:62 > + ProvisionalPageProxy* m_provisionalPage = nullptr; Can we use `Optional<ProvisionalPageProxy>` instead of a raw pointer? Then we could simply `m_provisionalPage.reset()` instead of assigning it to `nullptr` in `InspectorTargetProxy::didCommitProvisionalTarget`. Style: please use uniform initialization syntax ```cpp ProvisionalPageProxy* m_provisionalPage { nullptr }; ``` > Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:96 > + m_page.inspectorController().didDestroyProvisionalPage(*this, m_wasCommitted); Rather than go through the trouble of telling Web Inspector about this even when `m_wasCommitted` (which we then use to immediately return inside `WebPageInspectorController::didDestroyProvisionalPage`), can we just move this call below the early-return `if` below? Also, should this be the first call inside this function? That's usually where we normally put these types of lifecycle things (the `didCreate*` is the last line and the `willDestroy*` is the first line). > Source/WebKit/UIProcess/ProvisionalPageProxy.h:67 > - WebPageProxy& page() { return m_page; } > + WebPageProxy& page() const { return m_page; } Why was this changed? It seems like it's because `WebPageInspectorController::didDestroyProvisionalPage` requires a `const WebCore::PageIdentifier&`, but is there a reason why it has to be that way? Can you just pass it as a non-`const` reference? > Source/WebKit/UIProcess/WebPageInspectorController.cpp:61 > + String pageTargetId = WebPageInspectorTarget::toTargetID(m_page.webPageID()); > + createInspectorTarget(pageTargetId, Inspector::InspectorTargetType::Page); I think we still want this to be at the very end of `WebPageProxy::WebPageProxy`, to ensure that we're not trying to access any uninitialized data. > Source/WebKit/UIProcess/WebPageInspectorController.cpp:194 > + m_targetAgent->didCommitProvisionalTarget(oldID, newID); > + // We've disconnected from the old page and will not receive any message from it, so Style: missing newline > Source/WebKit/UIProcess/WebPageInspectorController.h:69 > + void didDestroyProvisionalPage(const ProvisionalPageProxy&, bool wasCommitted); Our normal "lifecycle" function names are `didCreate*` and `willDestroy*`. Technically, when `didDestroyProvisionalPage` is called, the `ProvisionalPageProxy` hasn't actually been destructed yet, so it's not entirely accurate to say that we "did" destroy it. > Source/WebKit/UIProcess/WebPageInspectorController.h:71 > + void didCommitProvisionalPage(const WebCore::PageIdentifier& oldWebPageID, const WebCore::PageIdentifier& newWebPageID); These probably shouldn't be references, as `PageIdentifier` is basically just a `uint64_t`. Other cases of `PageIdentifier` as a parameter also are not references. > Source/WebKit/UIProcess/WebPageInspectorController.h:81 > + HashMap<String, std::unique_ptr<InspectorTargetProxy>> m_targets; Could we use `WTF::UniqueRef` instead? I don't think we ever want a situation where we'd have a `nullptr` value in this map. > Source/WebKit/UIProcess/WebPageProxy.cpp:2983 > + const auto oldWebPageID = m_webPageID; Ditto (+WebPageInspectorController.h:71), specifically to the drop the `const`. > Source/WebKit/WebProcess/WebPage/WebPageInspectorTarget.cpp:50 > + if (m_channel) Should we `ASSERT(!m_channel);` to make sure that we're not trying to connect to a target more than once? > Source/WebKit/WebProcess/WebPage/WebPageInspectorTarget.cpp:72 > +String WebPageInspectorTarget::toTargetID(const WebCore::PageIdentifier& pageID) Ditto (+WebPageInspectorController.h:71) > Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetFrontendChannel.h:41 > + ~WebPageInspectorTargetFrontendChannel() override = default; `virtual ~WebPageInspectorTargetFrontendChannel() = default;` > LayoutTests/ChangeLog:13 > + * http/tests/inspector/target/target-events-for-proisional-page-expected.txt: Added. > + * http/tests/inspector/target/target-events-for-proisional-page.html: Added. Typo: "proisional" => "provisional" > LayoutTests/http/tests/inspector/target/target-events-for-proisional-page-expected.txt:8 > +PASS: received targetCreated event. We try to phrase our test messages as a "Should ___." (e.g. `Should receive `targetCreated` event."), as well as using complete sentences. > LayoutTests/http/tests/inspector/target/target-events-for-proisional-page-expected.txt:9 > +PASS: target is provisional. e.g. "Target should be provisional." > LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:10 > + let suite = InspectorTest.createAsyncSuite("Target.PSON"); > + suite.addTestCase({ Style: missing newline > LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:14 > + const targetAgent = InspectorBackend.domains.Target; `InspectorBackend.domains` exists only for feature checking purposes. It's not meant to be used for actual invocations of commands. I think a clearer approach to this would be to instead make your own `TestPSONTargetManager` that extends from `WI.TargetManager` and just overrides the functions you care about. ```js class TestPSONTargetManager extends TargetManager { targetCreated(targetInfo) { newTargetId = targetInfo.targetId; InspectorTest.pass(`received ${prop} event.`); InspectorTest.expectTrue(targetInfo.isProvisional, "target is provisional."); InspectorTest.expectEqual(targetInfo.previousTargetId, mainTargetId, "previos target id is id of the current target."); super.targetCreated(targetInfo); } didCommitProvisionalTarget(previousTargetId, targetInfo) { InspectorTest.pass(`received ${prop} event.`); InspectorTest.expectEqual(previousTargetId, mainTargetId, "previous target is the current one."); InspectorTest.expectEqual(targetInfo.targetId, newTargetId, "committed target matches provisional target."); super.targetCreated(targetInfo); } targetDestroyed(targetId) { InspectorTest.pass(`received ${prop} event.`); InspectorTest.expectEqual(targetId, mainTargetId, "destroyed target is previos target."); super.targetCreated(targetInfo); } } let testPSONTargetManager = new TestPSONTargetManager; WI.managers.splice(WI.managers.indexOf(WI.targetManager), 1, testPSONTargetManager); WI.targetManager = new TestPSONTargetManager; ``` This still feels quite "hacky", but it's not a bad solution. Another option is to just modify the prototype functions directly (e.g. `WI.TargetManager.prototype.targetCreated`), or to add special event dispatches that are only fired for tests. > LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:25 > + InspectorTest.expectEqual(targetInfo.previousTargetId, mainTargetId, "previos target id is id of the current target."); Typo: "previos" => "previous" > LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:40 > + InspectorTest.expectEqual(targetId, mainTargetId, "destroyed target is previos target."); Ditto > LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:61 > + 1. Target.targetCreated for provisional page. > + 2. Target.didCommitProvisionalTarget when provisional load is committed. > + 3. Target.targetDestroyed for the old target after the navigation request is committed. Please make these into separate `<p>` so it is printed somewhat nicely in the expected results file. > LayoutTests/platform/wk2/TestExpectations:767 > +# Target domain is only present in WebKit2. NIT: just `WK2` is fine :)
Comment on attachment 379967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379967&action=review >> Source/WebCore/ChangeLog:11 >> + (WebCore::InspectorController::setIsUnderTest): This method maybe closed several times > > Typo: "maybe closed" => "may be called" Done. >> Source/WebInspectorUI/ChangeLog:9 >> + no changes frontend bhavior, it will wait for the provisional target to commit > > Typo: "bhavior" => "behavior" Done. >> Source/WebInspectorUI/ChangeLog:25 >> + send commands to the provisional targtets yet, it has to ignore all activities > > Typo: "targtets" => "targets" Done. >> Source/WebInspectorUI/ChangeLog:30 >> + Better support for provisional targets will be added to frontend in a separate change. > > Can we have bug URL for this? I was going to use the same https://webkit.org/b/202219, so that it's easier to track what's going on across multiple patches in a single bug. >> Source/WebInspectorUI/ChangeLog:34 >> + of tasrgetDestroyed/targetCreated events which matches previous behavior. > > Typo: "tasrgetDestroyed" => "targetDestroyed" Done. >> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:139 >> + return; > > Style: please add a newline after this return Done. >> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:140 >> + InspectorTarget* target = m_targets.get(committedTargetID); > > NIT: `auto*` Done. >> Source/JavaScriptCore/inspector/agents/InspectorTargetAgent.cpp:142 >> + return; > > Ditto (139) Done. >> Source/JavaScriptCore/inspector/protocol/Target.json:11 >> + { "name": "isProvisional", "type": "boolean", "optional": true, "description": "Whether this is a provisional page target." }, > > NIT: I prefer putting `optional` values at the end of a type's declaration, so it's easier to read what's required vs what's optional Done. >> Source/JavaScriptCore/inspector/protocol/Target.json:12 >> + { "name": "previousTargetId", "type": "string", "optional": true }, > > Ditto > > Also, where is this actually used? I don't see the frontend using this value at all. It uses `oldTargetId` from `Target.event.didCommitProvisionalTarget` instead. Can they be merged? I'd rather have the `oldTargetId` parameter than add another optional value to `Target.TargetInfo`. I removed oldTargetId parameters in favor of TargetInfo.previousTargetId. I couldn't decide first between adding it as a field to the info structure and passing as a separate parameter in events. After talking to other folks about the experience with DevTools procol it seems better to just put it into the structure passed as a parameter in different target related events. It makes receiver's code more consistent and keeps the signatures of c++ classes sane. >> Source/WebCore/inspector/InspectorController.cpp:346 >> void InspectorController::setIsUnderTest(bool value) > > NIT: we could inline this in the header now that it's so small/simple Done. >> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:311 >> // Ignore errors if a worker went away quickly. > > NIT: please move this comment to be above the `if` so it's clear what it's referring to Done. >> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:312 >> + WorkerAgent.sendMessageToWorker(this._workerId, message).catch(e => { > > Style: please use a full/descriptive parameter name, like `error` > Style: we always add the `(...)` around the parameters of an arrow function, even if there's only one argument > ```js > WorkerAgent.sendMessageToWorker(this._workerId, message).catch((error) => { > ``` Done. >> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:313 >> + if (this.target.isDestroyed()) > > If the target is already destroyed, shouldn't we not even be sending the command to begin with? We should probably add an assertion somewhere `console.assert(!this.target.isDestroyed);`. > > Or is this to handle the situation where the target is destroyed after the command is dispatched, but before the response is received? It's the latter. >> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:315 >> + throw e; > > Rather than re-throw, please use `WI.reportInternalException(error)` instead. We try to avoid using `throw` as much as possible. Done. >> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:339 >> + TargetAgent.sendMessageToTarget(this._targetId, message).catch(e => { > > Ditto Done. >> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:342 >> + if (this.target.isDestroyed()) > > Ditto Done. >> Source/WebInspectorUI/UserInterface/Protocol/Connection.js:344 >> + throw e; > > Ditto Done. >> Source/WebInspectorUI/UserInterface/Protocol/Target.js:208 >> + isDestroyed() > > Why not use a getter? > ```js > get isDestroyed() { return this._isDestroyed; } > ``` Done. Normally I'd avoid getters/setters as the may hide side effects of seemingly innocent field access. It breaks the principle of least surprise. Basically with a method call it's obvious that it may run some logic while member access is normally expected to be a simple cheap operation with no extra behavior. I see that front-end uses getters/setters a lot so I'll just follow the same style. >> Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:32 >> + this._swappedTargetIds = new Set; > > Please move this logic to `WI.TargetManager`. We try to keep our observer classes (which are responsible for handling events received by the frontend) as "clean" of logic as possible, and instead have the various managers and model objects handle the actual logic. Done. All methods of TargetOberver are pure delegates to a singleton instance of TargetManager, what's the purpose of this class? Why not register TargetManager as event handler directly? >> Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:37 >> + if (targetInfo.isProvisional) { > > Please add a compatibility statement. > ``` > // COMPATIBILITY (iOS 13.0): `Target.TargetInfo.isProvisional` did not exist yet. Done. >> Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:44 >> + didCommitProvisionalTarget(previousTargetId, targetInfo) > > Along the lines of 30-31, I'd create a `WI.TargetManager.prototype.didCommitProvisionalTarget` function and move this logic inside of that. I'd strongly prefer keeping the name the same, so it's easier to follow the code through the various layers/transformations. Done. >> Source/WebInspectorUI/UserInterface/Test/Test.js:107 >> WI.transitionPageTarget = function(target) > > It seems like we should move these functions to `WI.TargetManager` so they don't get out of sync between 'Main.js' and 'Test.js'. Done. >> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:47 >> + auto target = create(provisionalPage.page(), targetId, type); > > NIT: I think for the purposes of code searching, I'd rather you also have the class name here too: > ```cpp > auto target = InspectorTargetProxy::create(provisionalPage.page(), targetId, type); > ``` Done. But it'd be better to improve the tooling so that it understands such calls :) >> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:65 >> if (m_page.hasRunningProcess()) > > Style: missing newline (since the previous `if` has an early return) Done. >> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:75 >> if (m_page.hasRunningProcess()) > > Ditto Done. >> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:85 >> if (m_page.hasRunningProcess()) > > Ditto Done. >> Source/WebKit/UIProcess/InspectorTargetProxy.h:51 >> + bool isProvisional() const override; > > NIT: these could all be `final` instead of `override` (or even omitted, since the class is marked `final`). This contradicts webkit.org/b/200959#c13, let's agree on the approach to override/final over there and I can update this patch accordingly. Omitting override/final altogether causes clang warnings as I mentioned in that bug and there is an outstanding patch generated via clang-tidy that fixes the issue (see https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html#modernize-use-override). >> Source/WebKit/UIProcess/InspectorTargetProxy.h:62 >> + ProvisionalPageProxy* m_provisionalPage = nullptr; > > Can we use `Optional<ProvisionalPageProxy>` instead of a raw pointer? Then we could simply `m_provisionalPage.reset()` instead of assigning it to `nullptr` in `InspectorTargetProxy::didCommitProvisionalTarget`. > > Style: please use uniform initialization syntax > ```cpp > ProvisionalPageProxy* m_provisionalPage { nullptr }; > ``` Updated the initializer. I believe you meant Optional<ProvisionalPageProxy&> as this class doesn't own ProvisionalPageProxy. What is the advantage of using more verbose Optional<ProvisionalPageProxy&> than a raw pointer here? WebKit style guide doesn't provide any specific recommendations about it. >> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:96 >> + m_page.inspectorController().didDestroyProvisionalPage(*this, m_wasCommitted); > > Rather than go through the trouble of telling Web Inspector about this even when `m_wasCommitted` (which we then use to immediately return inside `WebPageInspectorController::didDestroyProvisionalPage`), can we just move this call below the early-return `if` below? > > Also, should this be the first call inside this function? That's usually where we normally put these types of lifecycle things (the `didCreate*` is the last line and the `willDestroy*` is the first line). I tried to keep create/destroy notifications balanced no regardless of the provisional navigation outcome. That's the reason didDestroyProvisionalPage was before 'if (m_wasCommitted)' check. Updated the patch to only call didDestroyProvisionalPage for not committed pages. >> Source/WebKit/UIProcess/ProvisionalPageProxy.h:67 >> + WebPageProxy& page() const { return m_page; } > > Why was this changed? It seems like it's because `WebPageInspectorController::didDestroyProvisionalPage` requires a `const WebCore::PageIdentifier&`, but is there a reason why it has to be that way? Can you just pass it as a non-`const` reference? This is exactly as you described. The argument of didDestroyProvisionalPage is marked const as I'd like to indicate that it does't change its parameter. More generally marking a method as const if it's known to not change the instance's state gives the compiler more room for optimization. >> Source/WebKit/UIProcess/WebPageInspectorController.cpp:61 >> + createInspectorTarget(pageTargetId, Inspector::InspectorTargetType::Page); > > I think we still want this to be at the very end of `WebPageProxy::WebPageProxy`, to ensure that we're not trying to access any uninitialized data. This is how this code was already written I'm not changing it. If your goal is to protect against access to a partially initialized instance of WebPageProxy it should not be passed to the WebPageInspectorController until it's sufficiently initialized. Do you you prefer moving WebPageInspectorController call from member initialization in WebPageProxy to the end of the constructor or introducing separate method WebPageInspectorController::init(WebPageProxy& page) ? The latter means that the member pointing to WebPageProxy cannot be a reference anymore. >> Source/WebKit/UIProcess/WebPageInspectorController.cpp:194 >> + // We've disconnected from the old page and will not receive any message from it, so > > Style: missing newline Done. >> Source/WebKit/UIProcess/WebPageInspectorController.h:69 >> + void didDestroyProvisionalPage(const ProvisionalPageProxy&, bool wasCommitted); > > Our normal "lifecycle" function names are `didCreate*` and `willDestroy*`. Technically, when `didDestroyProvisionalPage` is called, the `ProvisionalPageProxy` hasn't actually been destructed yet, so it's not entirely accurate to say that we "did" destroy it. Done. >> Source/WebKit/UIProcess/WebPageInspectorController.h:71 >> + void didCommitProvisionalPage(const WebCore::PageIdentifier& oldWebPageID, const WebCore::PageIdentifier& newWebPageID); > > These probably shouldn't be references, as `PageIdentifier` is basically just a `uint64_t`. Other cases of `PageIdentifier` as a parameter also are not references. Yeah, it's used as a value type. Updated. >> Source/WebKit/UIProcess/WebPageInspectorController.h:81 >> + HashMap<String, std::unique_ptr<InspectorTargetProxy>> m_targets; > > Could we use `WTF::UniqueRef` instead? I don't think we ever want a situation where we'd have a `nullptr` value in this map. I can't seem to be able to use UniqueRef as a type of HashMap values because emptyValue() is not defined for it: DerivedSources/ForwardingHeaders/wtf/HashTraits.h:67:36: error: no matching constructor for initialization of 'WTF::UniqueRef<WebKit::InspectorTargetProxy>' static T emptyValue() { return T(); } ^ DerivedSources/ForwardingHeaders/wtf/HashTraits.h:78:55: note: in instantiation of member function 'WTF::GenericHashTraits<WTF::UniqueRef<WebKit::InspectorTargetProxy> >::emptyValue' requested here new (NotNull, std::addressof(slot)) T(Traits::emptyValue()); Also I don't see any examples in the code where UniqueRef would be used to store map values. >> Source/WebKit/UIProcess/WebPageProxy.cpp:2983 >> + const auto oldWebPageID = m_webPageID; > > Ditto (+WebPageInspectorController.h:71), specifically to the drop the `const`. This is different as it's not a reference here. I'd like to make sure oldWebPageID is not modified accidentally before it is passed to the inspector controller. >> Source/WebKit/WebProcess/WebPage/WebPageInspectorTarget.cpp:50 >> + if (m_channel) > > Should we `ASSERT(!m_channel);` to make sure that we're not trying to connect to a target more than once? I believe in the current implementation it's possible that connect() is called more than once with different front-end types. >> Source/WebKit/WebProcess/WebPage/WebPageInspectorTarget.cpp:72 >> +String WebPageInspectorTarget::toTargetID(const WebCore::PageIdentifier& pageID) > > Ditto (+WebPageInspectorController.h:71) Done. >> Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetFrontendChannel.h:41 >> + ~WebPageInspectorTargetFrontendChannel() override = default; > > `virtual ~WebPageInspectorTargetFrontendChannel() = default;` Why? It overrides virtual destructor in its superclass. >> LayoutTests/ChangeLog:13 >> + * http/tests/inspector/target/target-events-for-proisional-page.html: Added. > > Typo: "proisional" => "provisional" Done. >> LayoutTests/http/tests/inspector/target/target-events-for-proisional-page-expected.txt:8 >> +PASS: received targetCreated event. > > We try to phrase our test messages as a "Should ___." (e.g. `Should receive `targetCreated` event."), as well as using complete sentences. Done. >> LayoutTests/http/tests/inspector/target/target-events-for-proisional-page-expected.txt:9 >> +PASS: target is provisional. > > e.g. "Target should be provisional." Done. >> LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:10 >> + suite.addTestCase({ > > Style: missing newline Done. >> LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:14 >> + const targetAgent = InspectorBackend.domains.Target; > > `InspectorBackend.domains` exists only for feature checking purposes. It's not meant to be used for actual invocations of commands. > > I think a clearer approach to this would be to instead make your own `TestPSONTargetManager` that extends from `WI.TargetManager` and just overrides the functions you care about. > ```js > class TestPSONTargetManager extends TargetManager { > targetCreated(targetInfo) { > newTargetId = targetInfo.targetId; > InspectorTest.pass(`received ${prop} event.`); > InspectorTest.expectTrue(targetInfo.isProvisional, "target is provisional."); > InspectorTest.expectEqual(targetInfo.previousTargetId, mainTargetId, "previos target id is id of the current target."); > > super.targetCreated(targetInfo); > } > > didCommitProvisionalTarget(previousTargetId, targetInfo) { > InspectorTest.pass(`received ${prop} event.`); > InspectorTest.expectEqual(previousTargetId, mainTargetId, "previous target is the current one."); > InspectorTest.expectEqual(targetInfo.targetId, newTargetId, "committed target matches provisional target."); > > super.targetCreated(targetInfo); > } > > targetDestroyed(targetId) { > InspectorTest.pass(`received ${prop} event.`); > InspectorTest.expectEqual(targetId, mainTargetId, "destroyed target is previos target."); > > super.targetCreated(targetInfo); > } > } > > let testPSONTargetManager = new TestPSONTargetManager; > WI.managers.splice(WI.managers.indexOf(WI.targetManager), 1, testPSONTargetManager); > WI.targetManager = new TestPSONTargetManager; > ``` > > This still feels quite "hacky", but it's not a bad solution. Another option is to just modify the prototype functions directly (e.g. `WI.TargetManager.prototype.targetCreated`), or to add special event dispatches that are only fired for tests. I don't like the idea of deriving from TargetManager which is called indirectly when dispatching protocol commands, I'd like to be as close as possible to the target domain handler. Ideally this would be a protocol test if it didn't depend on the dummy page and supported target domain but this is a different problem. I changed the code to use TargetAgent similar to how other tests use *Agents. I hope this will be changed to a clean protocol test eventually. >> LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:25 >> + InspectorTest.expectEqual(targetInfo.previousTargetId, mainTargetId, "previos target id is id of the current target."); > > Typo: "previos" => "previous" Done. >> LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:40 >> + InspectorTest.expectEqual(targetId, mainTargetId, "destroyed target is previos target."); > > Ditto Done. >> LayoutTests/http/tests/inspector/target/target-events-for-proisional-page.html:61 >> + 3. Target.targetDestroyed for the old target after the navigation request is committed. > > Please make these into separate `<p>` so it is printed somewhat nicely in the expected results file. Done. >> LayoutTests/platform/wk2/TestExpectations:767 >> +# Target domain is only present in WebKit2. > > NIT: just `WK2` is fine :) Most of the other comments in this file use WebKit2 so I'd rather be consistent.
Created attachment 380442 [details] Patch
Comment on attachment 379967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379967&action=review >>> Source/WebInspectorUI/ChangeLog:30 >>> + Better support for provisional targets will be added to frontend in a separate change. >> >> Can we have bug URL for this? > > I was going to use the same https://webkit.org/b/202219, so that it's easier to track what's going on across multiple patches in a single bug. I'd prefer if you created separate bugs for each and have a blocking/blocked-by hierarchy. It can be confusing to have multiple unrelated patches come from the same URL/title. >>> Source/JavaScriptCore/inspector/protocol/Target.json:12 >>> + { "name": "previousTargetId", "type": "string", "optional": true }, >> >> Ditto >> >> Also, where is this actually used? I don't see the frontend using this value at all. It uses `oldTargetId` from `Target.event.didCommitProvisionalTarget` instead. Can they be merged? I'd rather have the `oldTargetId` parameter than add another optional value to `Target.TargetInfo`. > > I removed oldTargetId parameters in favor of TargetInfo.previousTargetId. I couldn't decide first between adding it as a field to the info structure and passing as a separate parameter in events. After talking to other folks about the experience with DevTools procol it seems better to just put it into the structure passed as a parameter in different target related events. It makes receiver's code more consistent and keeps the signatures of c++ classes sane. NIT: I personally would've preferred keeping it as an additional parameter to `didCommitProvisionalTarget`, as it keeps `TargetInfo` more general and clean of event-specific information. >>> Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:32 >>> + this._swappedTargetIds = new Set; >> >> Please move this logic to `WI.TargetManager`. We try to keep our observer classes (which are responsible for handling events received by the frontend) as "clean" of logic as possible, and instead have the various managers and model objects handle the actual logic. > > Done. All methods of TargetOberver are pure delegates to a singleton instance of TargetManager, what's the purpose of this class? Why not register TargetManager as event handler directly? Observers are created per-target, whereas the managers are global singletons. Even though the observers are basically pure-delegates, they do often handle some of the compatibility work, or are responsible for telling the manager what target the event came from (see `WI.DebuggerObserver` for examples). >>> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:47 >>> + auto target = create(provisionalPage.page(), targetId, type); >> >> NIT: I think for the purposes of code searching, I'd rather you also have the class name here too: >> ```cpp >> auto target = InspectorTargetProxy::create(provisionalPage.page(), targetId, type); >> ``` > > Done. But it'd be better to improve the tooling so that it understands such calls :) That puts the onus on the tooling. As an example, I sometimes use basic `grep` (or `ack`) to quickly search through the codebase from terminal. I shouldn't have to use some "special" tool in order to do basic searching of things. >>> Source/WebKit/UIProcess/InspectorTargetProxy.h:62 >>> + ProvisionalPageProxy* m_provisionalPage = nullptr; >> >> Can we use `Optional<ProvisionalPageProxy>` instead of a raw pointer? Then we could simply `m_provisionalPage.reset()` instead of assigning it to `nullptr` in `InspectorTargetProxy::didCommitProvisionalTarget`. >> >> Style: please use uniform initialization syntax >> ```cpp >> ProvisionalPageProxy* m_provisionalPage { nullptr }; >> ``` > > Updated the initializer. > > I believe you meant Optional<ProvisionalPageProxy&> as this class doesn't own ProvisionalPageProxy. What is the advantage of using more verbose Optional<ProvisionalPageProxy&> than a raw pointer here? WebKit style guide doesn't provide any specific recommendations about it. WebKit as a whole is trying to move away from using raw pointers wherever possible. Frankly, I'd be even happier to use `WeakPtr<ProvisionalPageProxy>` so we have greater safeguards against UAF, but Web Inspector is unlikely to run into these issues because of our instrumentation points being in the right places. >>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:61 >>> + createInspectorTarget(pageTargetId, Inspector::InspectorTargetType::Page); >> >> I think we still want this to be at the very end of `WebPageProxy::WebPageProxy`, to ensure that we're not trying to access any uninitialized data. > > This is how this code was already written I'm not changing it. If your goal is to protect against access to a partially initialized instance of WebPageProxy it should not be passed to the WebPageInspectorController until it's sufficiently initialized. Do you you prefer moving WebPageInspectorController call from member initialization in WebPageProxy to the end of the constructor or introducing separate method WebPageInspectorController::init(WebPageProxy& page) ? The latter means that the member pointing to WebPageProxy cannot be a reference anymore. Yes the code may be the same, but where you're calling it from is vastly different. The previous location of this was in `WebPageProxy::createInspectorTargets`, which was called at the very end of `WebPageProxy::WebPageProxy`. This is now called halfway through that function, as part of `m_inspectorController`'s initialization. That difference could cause problems later down the line for other developers who are unaware of Web Inspector specific code, so I'd prefer if we kept the location to be functionally equivalent to what it was before, likely by just calling this code at the end of `WebPageProxy::WebPageProxy` (or restoring `WebPageProxy::createInspectorTargets`). >>> Source/WebKit/UIProcess/WebPageInspectorController.h:81 >>> + HashMap<String, std::unique_ptr<InspectorTargetProxy>> m_targets; >> >> Could we use `WTF::UniqueRef` instead? I don't think we ever want a situation where we'd have a `nullptr` value in this map. > > I can't seem to be able to use UniqueRef as a type of HashMap values because emptyValue() is not defined for it: > > DerivedSources/ForwardingHeaders/wtf/HashTraits.h:67:36: error: no matching constructor for initialization of 'WTF::UniqueRef<WebKit::InspectorTargetProxy>' > static T emptyValue() { return T(); } > ^ > DerivedSources/ForwardingHeaders/wtf/HashTraits.h:78:55: note: in instantiation of member function 'WTF::GenericHashTraits<WTF::UniqueRef<WebKit::InspectorTargetProxy> >::emptyValue' requested here > new (NotNull, std::addressof(slot)) T(Traits::emptyValue()); > > Also I don't see any examples in the code where UniqueRef would be used to store map values. Ah, right, there's no empty value for a `UniqueRef`. I suppose this is fine then. >>> Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetFrontendChannel.h:41 >>> + ~WebPageInspectorTargetFrontendChannel() override = default; >> >> `virtual ~WebPageInspectorTargetFrontendChannel() = default;` > > Why? It overrides virtual destructor in its superclass. We have never done this anywhere in WebKit. It's also a bit weird, because this doesn't "technically" override the destructor, as the super class' destructor will still get called.
Comment on attachment 379967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379967&action=review >>>> Source/WebInspectorUI/ChangeLog:30 >>>> + Better support for provisional targets will be added to frontend in a separate change. >>> >>> Can we have bug URL for this? >> >> I was going to use the same https://webkit.org/b/202219, so that it's easier to track what's going on across multiple patches in a single bug. > > I'd prefer if you created separate bugs for each and have a blocking/blocked-by hierarchy. It can be confusing to have multiple unrelated patches come from the same URL/title. Filed https://bugs.webkit.org/show_bug.cgi?id=202704 >>>> Source/JavaScriptCore/inspector/protocol/Target.json:12 >>>> + { "name": "previousTargetId", "type": "string", "optional": true }, >>> >>> Ditto >>> >>> Also, where is this actually used? I don't see the frontend using this value at all. It uses `oldTargetId` from `Target.event.didCommitProvisionalTarget` instead. Can they be merged? I'd rather have the `oldTargetId` parameter than add another optional value to `Target.TargetInfo`. >> >> I removed oldTargetId parameters in favor of TargetInfo.previousTargetId. I couldn't decide first between adding it as a field to the info structure and passing as a separate parameter in events. After talking to other folks about the experience with DevTools procol it seems better to just put it into the structure passed as a parameter in different target related events. It makes receiver's code more consistent and keeps the signatures of c++ classes sane. > > NIT: I personally would've preferred keeping it as an additional parameter to `didCommitProvisionalTarget`, as it keeps `TargetInfo` more general and clean of event-specific information. This field is now used in all events where TargetInfo is passed as parameter so I'd rather keep it inside the structure. Let me know if it doesn't make sense to you. >>>> Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:32 >>>> + this._swappedTargetIds = new Set; >>> >>> Please move this logic to `WI.TargetManager`. We try to keep our observer classes (which are responsible for handling events received by the frontend) as "clean" of logic as possible, and instead have the various managers and model objects handle the actual logic. >> >> Done. All methods of TargetOberver are pure delegates to a singleton instance of TargetManager, what's the purpose of this class? Why not register TargetManager as event handler directly? > > Observers are created per-target, whereas the managers are global singletons. Even though the observers are basically pure-delegates, they do often handle some of the compatibility work, or are responsible for telling the manager what target the event came from (see `WI.DebuggerObserver` for examples). Do they ever store any state except for the target they are attached to? I'm thinking about the case when such manager would have to listen to provisional and current targets at the same time and keep their states separately somehow. This would essentially mean that each bit of the state should be associated with a target. >>>> Source/WebKit/UIProcess/InspectorTargetProxy.cpp:47 >>>> + auto target = create(provisionalPage.page(), targetId, type); >>> >>> NIT: I think for the purposes of code searching, I'd rather you also have the class name here too: >>> ```cpp >>> auto target = InspectorTargetProxy::create(provisionalPage.page(), targetId, type); >>> ``` >> >> Done. But it'd be better to improve the tooling so that it understands such calls :) > > That puts the onus on the tooling. As an example, I sometimes use basic `grep` (or `ack`) to quickly search through the codebase from terminal. I shouldn't have to use some "special" tool in order to do basic searching of things. Yeah, this is what I'm saying, it's 21 century and it'd be nice to have an IDE that supported nice code navigation rather than relying on plain text search. But there is still no good enough solution for c++ :) >>>> Source/WebKit/UIProcess/InspectorTargetProxy.h:62 >>>> + ProvisionalPageProxy* m_provisionalPage = nullptr; >>> >>> Can we use `Optional<ProvisionalPageProxy>` instead of a raw pointer? Then we could simply `m_provisionalPage.reset()` instead of assigning it to `nullptr` in `InspectorTargetProxy::didCommitProvisionalTarget`. >>> >>> Style: please use uniform initialization syntax >>> ```cpp >>> ProvisionalPageProxy* m_provisionalPage { nullptr }; >>> ``` >> >> Updated the initializer. >> >> I believe you meant Optional<ProvisionalPageProxy&> as this class doesn't own ProvisionalPageProxy. What is the advantage of using more verbose Optional<ProvisionalPageProxy&> than a raw pointer here? WebKit style guide doesn't provide any specific recommendations about it. > > WebKit as a whole is trying to move away from using raw pointers wherever possible. Frankly, I'd be even happier to use `WeakPtr<ProvisionalPageProxy>` so we have greater safeguards against UAF, but Web Inspector is unlikely to run into these issues because of our instrumentation points being in the right places. Using WeakPtr does make sense here as it protects against UAF, thanks for the pointer! I didn't know that WebKit was trying to use it. Also there is nothing special about inspector that would make its code less susceptible to this sort of bugs compared to other components. Replaced raw pointer with WeakPtr. It may also make sense to update references to WebPageProxy from inspector structures to use WeakPtr as well. WDYT? >>>> Source/WebKit/UIProcess/WebPageInspectorController.cpp:61 >>>> + createInspectorTarget(pageTargetId, Inspector::InspectorTargetType::Page); >>> >>> I think we still want this to be at the very end of `WebPageProxy::WebPageProxy`, to ensure that we're not trying to access any uninitialized data. >> >> This is how this code was already written I'm not changing it. If your goal is to protect against access to a partially initialized instance of WebPageProxy it should not be passed to the WebPageInspectorController until it's sufficiently initialized. Do you you prefer moving WebPageInspectorController call from member initialization in WebPageProxy to the end of the constructor or introducing separate method WebPageInspectorController::init(WebPageProxy& page) ? The latter means that the member pointing to WebPageProxy cannot be a reference anymore. > > Yes the code may be the same, but where you're calling it from is vastly different. The previous location of this was in `WebPageProxy::createInspectorTargets`, which was called at the very end of `WebPageProxy::WebPageProxy`. This is now called halfway through that function, as part of `m_inspectorController`'s initialization. That difference could cause problems later down the line for other developers who are unaware of Web Inspector specific code, so I'd prefer if we kept the location to be functionally equivalent to what it was before, likely by just calling this code at the end of `WebPageProxy::WebPageProxy` (or restoring `WebPageProxy::createInspectorTargets`). Moved this to init() method which is called when WebPageProxy is fully initialized. PTAL. >>>> Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetFrontendChannel.h:41 >>>> + ~WebPageInspectorTargetFrontendChannel() override = default; >>> >>> `virtual ~WebPageInspectorTargetFrontendChannel() = default;` >> >> Why? It overrides virtual destructor in its superclass. > > We have never done this anywhere in WebKit. It's also a bit weird, because this doesn't "technically" override the destructor, as the super class' destructor will still get called. Well, overriding method in general is free to call super class's implementation I don't see any issues with that. Here is an excerpt from the WebKit style guide (https://webkit.org/code-style-guidelines/#override-methods) which clearly says that overriding methods should be annotated as either override or final: "All subclasses of that class must either specify the override keyword when overriding the virtual method or the final keyword when overriding the virtual method and requiring that no further subclasses can override it." I believe the reason there is still a lot of code that violates the rule is that they predate c++11 usage in WebKit and nobody bothered updating them. Maybe worth bringing this up on webkit-dev. Annotating the destructor with override protects against a nasty error when super destructor accidentally not not marked as virtual but the clients try to destroy the object using its superclass interface.
Comment on attachment 379967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379967&action=review >>> Source/WebCore/inspector/InspectorController.cpp:346 >>> void InspectorController::setIsUnderTest(bool value) >> >> NIT: we could inline this in the header now that it's so small/simple > > Done. Actually this doesn't work: ERROR: Source/WebCore/inspector/InspectorController.h:104: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. [build/webcore_export] [4]
Comment on attachment 379967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379967&action=review >>>> Source/WebCore/inspector/InspectorController.cpp:346 >>>> void InspectorController::setIsUnderTest(bool value) >>> >>> NIT: we could inline this in the header now that it's so small/simple >> >> Done. > > Actually this doesn't work: > > ERROR: Source/WebCore/inspector/InspectorController.h:104: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. [build/webcore_export] [4] We can remove the macro if it's in the header. >>>>> Source/WebInspectorUI/UserInterface/Protocol/TargetObserver.js:32 >>>>> + this._swappedTargetIds = new Set; >>>> >>>> Please move this logic to `WI.TargetManager`. We try to keep our observer classes (which are responsible for handling events received by the frontend) as "clean" of logic as possible, and instead have the various managers and model objects handle the actual logic. >>> >>> Done. All methods of TargetOberver are pure delegates to a singleton instance of TargetManager, what's the purpose of this class? Why not register TargetManager as event handler directly? >> >> Observers are created per-target, whereas the managers are global singletons. Even though the observers are basically pure-delegates, they do often handle some of the compatibility work, or are responsible for telling the manager what target the event came from (see `WI.DebuggerObserver` for examples). > > Do they ever store any state except for the target they are attached to? I'm thinking about the case when such manager would have to listen to provisional and current targets at the same time and keep their states separately somehow. This would essentially mean that each bit of the state should be associated with a target. My changes in <https://webkit.org/b/200384> pass in both the associated agent and target, both of which are able to be used (if needed) by the various observer subclasses. One thing that's worth mentioning is that in the case of the `WI.TargetObserver`, the `this._target` would actually be the direct connection to the `WebPage`, so it likely wouldn't be needed for really anything. The `this._target` is more useful for the rest of the domains (see `WI.DebuggerObserver` for examples). >>>>> Source/WebKit/UIProcess/InspectorTargetProxy.h:62 >>>>> + ProvisionalPageProxy* m_provisionalPage = nullptr; >>>> >>>> Can we use `Optional<ProvisionalPageProxy>` instead of a raw pointer? Then we could simply `m_provisionalPage.reset()` instead of assigning it to `nullptr` in `InspectorTargetProxy::didCommitProvisionalTarget`. >>>> >>>> Style: please use uniform initialization syntax >>>> ```cpp >>>> ProvisionalPageProxy* m_provisionalPage { nullptr }; >>>> ``` >>> >>> Updated the initializer. >>> >>> I believe you meant Optional<ProvisionalPageProxy&> as this class doesn't own ProvisionalPageProxy. What is the advantage of using more verbose Optional<ProvisionalPageProxy&> than a raw pointer here? WebKit style guide doesn't provide any specific recommendations about it. >> >> WebKit as a whole is trying to move away from using raw pointers wherever possible. Frankly, I'd be even happier to use `WeakPtr<ProvisionalPageProxy>` so we have greater safeguards against UAF, but Web Inspector is unlikely to run into these issues because of our instrumentation points being in the right places. > > Using WeakPtr does make sense here as it protects against UAF, thanks for the pointer! I didn't know that WebKit was trying to use it. Also there is nothing special about inspector that would make its code less susceptible to this sort of bugs compared to other components. Replaced raw pointer with WeakPtr. > > It may also make sense to update references to WebPageProxy from inspector structures to use WeakPtr as well. WDYT? I think generally it's safer to have anything that uses a raw pointer use a `WeakPtr` (or a `RefPtr`, but that has other issues [1]) instead. That should likely be done in a separate patch though. [1]: Web Inspector sometimes can't make use of `Ref`/`RefPtr` because we don't want to change the lifecycle of the object we're inspecting (see `Node`, `WebSocket`, `CanvasRenderingContext`, `WebGLProgram`, `WebGPUDevice`, or `WebGPUPipeline` for examples). >>>>> Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetFrontendChannel.h:41 >>>>> + ~WebPageInspectorTargetFrontendChannel() override = default; >>>> >>>> `virtual ~WebPageInspectorTargetFrontendChannel() = default;` >>> >>> Why? It overrides virtual destructor in its superclass. >> >> We have never done this anywhere in WebKit. It's also a bit weird, because this doesn't "technically" override the destructor, as the super class' destructor will still get called. > > Well, overriding method in general is free to call super class's implementation I don't see any issues with that. > > Here is an excerpt from the WebKit style guide (https://webkit.org/code-style-guidelines/#override-methods) which clearly says that overriding methods should be annotated as either override or final: > "All subclasses of that class must either specify the override keyword when overriding the virtual method or the final keyword when overriding the virtual method and requiring that no further subclasses can override it." > > I believe the reason there is still a lot of code that violates the rule is that they predate c++11 usage in WebKit and nobody bothered updating them. Maybe worth bringing this up on webkit-dev. Annotating the destructor with override protects against a nasty error when super destructor accidentally not not marked as virtual but the clients try to destroy the object using its superclass interface. That may be the case, but for now I'd ask you to use `virtual`, as it seems like that's what we use everywhere else in WebKit. Personally, I would also argue that constructors and destructors are considered a different "type" of method than other member functions, so that affects my stance on this too. Feel free to broach the subject on webkit-dev.
Comment on attachment 379967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379967&action=review >>>>> Source/WebCore/inspector/InspectorController.cpp:346 >>>>> void InspectorController::setIsUnderTest(bool value) >>>> >>>> NIT: we could inline this in the header now that it's so small/simple >>> >>> Done. >> >> Actually this doesn't work: >> >> ERROR: Source/WebCore/inspector/InspectorController.h:104: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. [build/webcore_export] [4] > > We can remove the macro if it's in the header. Done. >>>>>> Source/WebKit/WebProcess/WebPage/WebPageInspectorTargetFrontendChannel.h:41 >>>>>> + ~WebPageInspectorTargetFrontendChannel() override = default; >>>>> >>>>> `virtual ~WebPageInspectorTargetFrontendChannel() = default;` >>>> >>>> Why? It overrides virtual destructor in its superclass. >>> >>> We have never done this anywhere in WebKit. It's also a bit weird, because this doesn't "technically" override the destructor, as the super class' destructor will still get called. >> >> Well, overriding method in general is free to call super class's implementation I don't see any issues with that. >> >> Here is an excerpt from the WebKit style guide (https://webkit.org/code-style-guidelines/#override-methods) which clearly says that overriding methods should be annotated as either override or final: >> "All subclasses of that class must either specify the override keyword when overriding the virtual method or the final keyword when overriding the virtual method and requiring that no further subclasses can override it." >> >> I believe the reason there is still a lot of code that violates the rule is that they predate c++11 usage in WebKit and nobody bothered updating them. Maybe worth bringing this up on webkit-dev. Annotating the destructor with override protects against a nasty error when super destructor accidentally not not marked as virtual but the clients try to destroy the object using its superclass interface. > > That may be the case, but for now I'd ask you to use `virtual`, as it seems like that's what we use everywhere else in WebKit. Personally, I would also argue that constructors and destructors are considered a different "type" of method than other member functions, so that affects my stance on this too. Feel free to broach the subject on webkit-dev. Raised this on webkit-dev, let's discuss there and I'll update the patch to follow the conclusion.