Skip to content
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

Fix reading property of null object in chrome extension's updateEmbedElement function #16612

Merged

Conversation

erm1116
Copy link
Contributor

@erm1116 erm1116 commented Jun 27, 2023

We have faced the problem that PDF Viewer extension does not work correctly in the following cases

<div>
  <embed src="..." />
</div>

The error output to the console is as follows

contentscript.js:136 Uncaught TypeError: Cannot read properties of null (reading 'before')
    at updateEmbedElement (contentscript.js:136:17)
    at updateViewerFrame (contentscript.js:99:11)
    at watchObjectOrEmbed (contentscript.js:108:3)
    at HTMLDocument.onAnimationStart (contentscript.js:33:5)

Checking the previous changes,
it looks like the change is from parentNode.insertBefore(elem, nextSibling) to nextSibling.before(elem).
In the case of insertBefore, if nextSibling is null, it is added as the end of the node's child, so it behaves differently.

Therefore, if nextSibling does not exist, adding the node to the end of the child by parentNode.append.

Reference:
Node.insertBefore()
Element.before()

@erm1116 erm1116 changed the title Fix reading property of null object in updateEmbedElement function Fix reading property of null object in chrome extension's updateEmbedElement function Jun 27, 2023
@Rob--W
Copy link
Member

Rob--W commented Jun 28, 2023

Thanks for this patch.

This might be the cause of #16594.

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have used the insertBefore instead of before/append because that method did exactly what it was supposed to. But this works too, so r+.

@erm1116
Copy link
Contributor Author

erm1116 commented Jun 28, 2023

Yes, I think insertBefore would work.
But since the Lint rule was set, I use before/append to solve this problem.

@erm1116 erm1116 force-pushed the fix-contentscript-updateEmbedElement branch from dce076e to fcf38f6 Compare June 30, 2023 13:39
@timvandermeij timvandermeij merged commit 73b6ee5 into mozilla:master Jul 1, 2023
@timvandermeij
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants