1857368 - DevTools does not refresh source files when reloading an extension

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P2)

Description

[Simeon Vincent [:dotproto]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:109.0) Gecko/20100101 Firefox/120.0

Steps to reproduce:

1. Unzip the attached extension and load it in Firefox as a temporary add-on.
2. Inspect the extension's background page and observe that the browser action listener will log the current timestamp.
3. Click the browser action and observe the messaged logged to the console in DevTools. For example, "1696548820167".
4. Open a text editor and modify the console.log() statement to include a second parameter, then save the file.
5. In Firefox, reload the extension. Note that DevTools still shows the original version of background.js.
6. Trigger the browser action again and observe that the statement logged in DevTools contains both a timestamp and a second value. For example, "1696548820167 another string".
7. Close the DevTools window for the extension.
8. Inspect the extension as you did in Step 2. Note that the source of background.js has been updated.

Actual results:

For the duration of a DevTools session, it will show the contents of the extension's directory at the time DevTools was opened. Reloading the extension in using about:debugging#/runtime/this-firefox, the Reload button in DevTools, or using a keyboard shortcut will not cause the source files to refresh.

Expected results:

DevTools should reflect the contents of the extension when it was last (re)loaded. If the developer has a source file open in DevTools and reload the extension, the view of that file should refresh to reflect the file's latest modifications.

Comment 1

[Simeon Vincent [:dotproto]]

Attachment: refresh-bug.zip

Comment 2

[Simeon Vincent [:dotproto]]

Attachment: A short video showing a reproduction of the issue.

Comment 3

[Simeon Vincent [:dotproto]]

This bug report may be a duplicate of 1352217. I filed it because it wasn't immediately clear to me whether that issue was the same or why work on it stopped roughly 7 years ago.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'WebExtensions::Untriaged' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 5

[Julian Descottes [:jdescottes]]

The toolbox reload button is only reloading the addon, but we don't try to refresh the content of the debugger or any other panel, and it seems that the current webextension target / descriptor is still alive and reused after the reload.

We should investigate if this behavior makes sense, and in any case force a refresh of the DevTools panel content.

This issue might be specific about background pages, which seems to be reused.

Luca: Quick question: I imagine the same thing can happen when webext triggers a reload? Did you implement any workaround to force DevTools to show up to date sources? Or has it not been reported.

Comment 6

[Alexandre Poirot [:ochameau] [PTO back August 21]]

When we click on the reload icon of DevTools, we end up calling this code:
https://searchfox.org/mozilla-central/rev/5ad226c7379b0564c76dc3b54b44985356f94c5a/devtools/server/actors/descriptors/webextension.js#198-212
It isn't clear to me what happens regarding the background page/script. Is it only reloaded or the <browser> element is destroyed?

Regarding devtools behavior, we may have issues because WebExtension debugging is still not using EveryFrameTarget (EFT) mode.
https://searchfox.org/mozilla-central/rev/5ad226c7379b0564c76dc3b54b44985356f94c5a/devtools/server/actors/descriptors/webextension.js#172-179
We aren't spawning one target per WindowGlobal. So if the background page element isn't destroyed and is only reloaded... we probably don't destroy the top level target and this explain why the source isn't reloaded.
If that's that. Then bug 1754452 may fix all of this. Unfortunately I've not been able to move forward with this, because it is interleaved with bug 1785266.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Alexandre Poirot [:ochameau] from comment #6)

When we click on the reload icon of DevTools, we end up calling this code:
https://searchfox.org/mozilla-central/rev/5ad226c7379b0564c76dc3b54b44985356f94c5a/devtools/server/actors/descriptors/webextension.js#198-212
It isn't clear to me what happens regarding the background page/script. Is it only reloaded or the <browser> element is destroyed?

The "extension reload" is basically an "extension shutdown + extension startup", and so the previous browser element where the background page was loaded is destroyed (during the extension shutdown phase) and a new one is then created (during the extension startup phase that follows). On the contrary the separate browser element that we use to attach the remote debugging to the right process is not destroyed (until the last use of it from the devtools internals releases it).

Regarding devtools behavior, we may have issues because WebExtension debugging is still not using EveryFrameTarget (EFT) mode.
https://searchfox.org/mozilla-central/rev/5ad226c7379b0564c76dc3b54b44985356f94c5a/devtools/server/actors/descriptors/webextension.js#172-179
We aren't spawning one target per WindowGlobal. So if the background page element isn't destroyed and is only reloaded... we probably don't destroy the top level target and this explain why the source isn't reloaded.
If that's that. Then bug 1754452 may fix all of this. Unfortunately I've not been able to move forward with this, because it is interleaved with bug 1785266.

As mentioned above the previous browser element associated to the background page is destroyed, but I need to refresh my memory about Bug 1754452 patch (and reading more carefully my last round of comments on that patch from quite some time ago may also help me to refresh my memory about it) and so I can't exclude yet that it may be part of how we may want to fix this issue.

Also I haven't yet tried to dig a bit more into what is going on internally on the devtools internals side when the issue is being hit, and so I'll keep this needinfo and come back to this after reproducing the issue locally and look into it more closely (and also confirm if I can trigger it only on mobile or both mobile and desktop scratch that, the mention to "desktop vs. mobile" was due to me mistakenly thinking to another unrelated addon toolbox issue reported around the same time).

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

I've looked into this a bit more closely locally and by inspecting the devtools server internals from a Browser Toolbox I have confirmed that the source actor for the new version of the background script file is actually being created and the source actor is being notified to the watcher registered from the debugger panel client, but then the new source actor received is still not reflected in the source code text shown in the debugger panel isn't getting updated (at the moment my guess is that the old sources would be expected to have been cleared from the redux store before the new source actor is being notified to the source watcher, but I haven't explicitly confirmed that yet).

If I'm reading and following the expected behavior of the debugger client's source and source-actors reducers, the issue seems to be related to the following difference between the webextensions target actor and other target actors like the tab target:

• for a tab target actor, (at least based on reading the code around the debugger client panel "sources"-related internals, and the sources redux reducer in particular) when the target tab is reloaded it seems we would be clearing all the sources and source actors when the target is being destroyed, we get to call onTargetDestroyed which then dispatches the "REMOVE_THREAD" action type and the sources and source-actors reducers will be clearing the sources and source actors from the redux store, then the new source resources will be notified and the source refreshed.

• for the webextensions target actor, we do use a browser element which is expected to don't be destroyed while the extension is being reloaded, and so when the browser element associated to the background page context is destroyed as part the extension being reloaded the target actor isn't being destroyed, onTargetDestroyed isn't being called and as a side effect of that the source and source-actors reducers will not be removing the old sources from the redux store

Hey Alex, does it (what I'm describing above) seem a reasonable explaination of what may not be working as expected?
(or is anything mentioned right above mismatching the actual expected behavior due to me misunderstanding what should be the expected behavior or misreading some of the related internals?)

Comment 9

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Seems to be a combination of various reasons which changed over time, as well as old design principles.

Bug 1814613 prevented emitting will-navigate for Web Extensions. will-navigate used to be the one event which would clear the whole debugger internal state, including sources.

But later, bug 1832585 revised this in favor of cleaning up debugger state only per target (per document or worker destruction). This allows to clear state when a worker or an iframe is destroyed and not only everything on navigation or reload. This was key for Service Worker debugging as they are kept alive across navigations. So it is no longer so important to have a functional will-navigate

Now the issue is that WebExtensions have always had a unique target, kept alive for the whole debugging session.
This is what we call internaly non-EFT (non EveryFrameTarget. We aren't getting one target per WindowGlobal).
The debugger would correctly clear itself if that target was destroyed when reloading the add-on.

As said in previous comment, bug 1754452 is a first step toward fixing this, but it is a challenging architectural change.
I'll investigate if we can revive this work, or find a workaround in the meantime.

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #8)

• for the webextensions target actor, we do use a browser element which is expected to don't be destroyed while the extension is being reloaded, and so when the browser element associated to the background page context is destroyed as part the extension being reloaded the target actor isn't being destroyed, onTargetDestroyed isn't being called and as a side effect of that the source and source-actors reducers will not be removing the old sources from the redux store

Hey Alex, does it (what I'm describing above) seem a reasonable explaination of what may not be working as expected?
(or is anything mentioned right above mismatching the actual expected behavior due to me misunderstanding what should be the expected behavior or misreading some of the related internals?)

Yes, you were on the right track. It is all about not destroying the target. Now, the source of trouble isn't the stable browser element, but rather the usage of connectToFrame. connectToFrame is the old deprecated way to attach to a browser element and was designed to work only against a unique target. Destroying the browser element wouldn't help as connectToFrame doesn't support the target-available/destroyed RDP events. These events are emitted by the Watcher Actor.
That's why, migrating WebExtension to be debugged via the Watcher Actor is the right way to address this issue.

Comment 10

[Alexandre Poirot [:ochameau] [PTO back August 21]]

This should be fixed by bug 1754452.

Comment 11

[Alexandre Poirot [:ochameau] [PTO back August 21]]

Let me know, via some new more specific bug, if bug 1754452 wasn't enough to address all the issues.