-
-
Notifications
You must be signed in to change notification settings - Fork 943
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SingleFile freezes when saving with Zotero Connector enabled #1479
Comments
Hi @adomasven, I confirm I can reproduce the issue. I've done a debugging session. The issue is related to the The problem is that this function is called very often, even if the parameters don't change. I was able to circumvent the issue by replacing the implementation with the code below (I've added a few _setFrameAttributes(attributes, style) {
for (let key in attributes) {
if (this._frame[key] !== attributes[key]) {
this._frame[key] = attributes[key];
}
}
if (this._frame.getAttribute("id") !== attributes.id) {
this._frame.setAttribute("id", attributes.id);
}
for (let key in style) {
if (this._frame.style) {
this._frame.style[key] = style[key];
}
}
} |
It looks like the real issue is that the observer is never disconnected here https://github.com/zotero/zotero-connectors/blob/2325e9d2b20c9f80639063d71997868535935451/src/browserExt/zoteroFrame.js#L65-L70. I don't see the exit condition. The other problem is that the infinite loop due to the recursive call is too tight. The |
Confirmed, when uninstalled the Zotero plugin, SingleFile plugin works ok. |
@gildas-lormeau thanks for taking a look. I guess SingleFile is setting some attributes on all iframes that it's capturing which is causing this to run in the first place? The code you've discovered was recently added to prevent websites from accidentally "tampering" with the Zotero translation sandbox frame as that was causing it to take up empty space on some websites, but in my testing it wasn't causing things to run in an infinite loop. Either way, this should be easily enough resolved on our end. |
Zotero Connector will be fixed in the next release, pending Chrome extension store approval. |
@adomasven Thank you. I confirm that SingleFile adds an attribute containing an id on all frames in order to recognize them when processing the page. Note that there is still a potential infinite loop here https://github.com/zotero/zotero-connectors/blob/b5cf8a0f326fc78ba4abcfa90b4cc9efd0a3ed57/src/browserExt/zoteroFrame.js#L66C4-L68 because |
Yes, that is intentional. The need to even have a mutation observer in the first place is because some websites change our iframes post page-load, and we can't know when, or whether they might decide to change them again (e.g. for navigation without page-load). I'm still not sure why SingleFile changing the id was causing an infinite loop. Is it repeatedly trying to change the ID too or something? |
It's true that when I think about it, I find it hard to understand why the listener was called following the modifications made by |
You might still want to consider not saving extension frames in snapshots, but that should probably be a separate issue. Thanks for the help! |
Hi gildas,
We at Zotero have recently updated the Zotero Connector to use MV3. As part of that change, we had to reimplement our page translation architecture to run in an sandboxed-eval iframe. We have had users report that having Zotero Connector enabled freezes saving with SingleFile, and I have confirmed that on the linked medium page. As the user reports, I indeed see some network activity related to our translation sandbox frame, and having inspected what's happening I have noticed that DOMContentLoaded event keeps firing on our frame, as if being refreshed.
I'm not sure if we're doing something wrong here, or if SingleFile is doing something weird with frames it saves (especially frames that belong to other extensions). Moreover, I'm not sure if SingleFile should be attempting to save frames from non https?:// origins, as it might cause issues with any other extension that uses the sandboxed eval frame too.
The text was updated successfully, but these errors were encountered: