1647894 - resource://gre/modules/HiddenFrame.jsm, line 23: TypeError: can't access property Symbol.iterator, ChromeUtils.nondeterministicGetWeakSetKeys(...) is undefined

Status: RESOLVED FIXED

(Toolkit :: General, defect, P5)

Description

[Rob Wu [:robwu]]

In a log of an unrelated bug (with a test failure), I spotted the following message in the log:

JavaScript error: resource://gre/modules/HiddenFrame.jsm, line 23: TypeError: can't access property Symbol.iterator, ChromeUtils.nondeterministicGetWeakSetKeys(...) is undefined

When I look at the code at https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/toolkit/modules/HiddenFrame.jsm#23
... then the failure reason is quite obvious: ChromeUtils.nondeterministicGetWeakSetKeys is used on gAllHiddenFrames, which is not a WeakSet but just a Set (so the return value is undefined instead of an array). This bug can be fixed by directly iterating over gAllHiddenFrames instead.

Comment 1

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

The severity field is not set for this bug.
:Dolske, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Cas Verploegen]

Hi. If I understand correctly, what you're saying is to basically change ChromeUtils.nondeterministicGetWeakSetKeys(gAllHiddenFrames) to gAllHiddenFrames. If that's all I'd like to have a go at this as my first bugfix!

Comment 3

[Rob Wu [:robwu]]

It's indeed that simple. To verify the fix, I suggest that you use Searchfox to find a test that triggers this error, and run that test before fixing this bug.

When you do observe the failure, fix the bug and run the test again. The reported error should be gone.

Comment 4

[Cas Verploegen]

It took some time to get up and running, I think everything works now. I ran into one problem, I am unable to find a test on Searchfox that triggers this exact error. Could you walk me through the steps or provide a link to the test? Thanks in advance. Furthermore, could I assign you as the reviewer once the patch is ready?

Comment 5

[Rob Wu [:robwu]]

To find a test that triggers the error, look up where ensureCleanupRegistered is called, and go all the way up. Ultimately it is called from the get method of HiddenFrame. So look for files that import/use HiddenFrame and call .get() on it.

Furthermore, could I assign you as the reviewer once the patch is ready?

Sure.

Comment 6

[Cas Verploegen]

Thanks, with this method I found the correct test to be located in testing/specialpowers/content/SpecialPowersParent.jsm (https://searchfox.org/mozilla-central/source/testing/specialpowers/content/SpecialPowersParent.jsm#1160). However, when trying to run the test with ./mach test testing/specialpowers/content/SpecialPowersParent.jsm I get the error message that it was unable to find tests from the given argument(s). I also tried running it with wpt, mochitest, xpcshell-test and reftest instead of test, but with no luck yet. Could you point me in the right direction on how to run the test correctly?

Comment 7

[Rob Wu [:robwu]]

SpecialPowersParent.jsm is not a test file, but an implementation of the test framework. Test files in Firefox are commonly stored in a directory (or subdirectory) of a directory called test.

I guess that you searched for HiddenFrame.get to find that file. The result is coincidentally correct, but only because that file decided to use a variable name with a similar name as the class instance that you're looking for (hiddenFrame).

To find better results, I suggest that you try the following:

• Find files that import HiddenFrame.jsm
• Find occurrences of the HiddenFrame constructor (which was imported from HiddenFrame.jsm).
• Look for .get() calls.

There aren't that many instances, so for this specific case you could just look at test files that import HiddenFrame.jsm, but nevertheless it would be a good practice to truly find where the code is being called.

Comment 8

[Cas Verploegen]

Okay, I looked again and found three instances of .get() calls on the HiddenFrame instance (https://searchfox.org/mozilla-central/source/browser/components/uitour/test/browser_no_tabs.js#24 , https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_webRequest.js#16 , https://searchfox.org/mozilla-central/source/browser/base/content/test/static/browser_parsable_css.js#357). However, when running these tests, the error does not occur in the output logs. Am I looking in the wrong place or is something else at play here?
For more info, I ran ./mach test browser/components/uitour/test/browser_no_tabs.js and similar commands for the other test files.

Comment 9

[Rob Wu [:robwu]]

The issue occurs at late shutdown, and I don't recall how to trigger it from a test.
Let's submit a patch without running automated tests then.

If you want to verify that the logic is correct without running tests, then you can open the global JavaScript console of Firefox (https://developer.mozilla.org/en-US/docs/Tools/Browser_Console) and run a code snippet to verify that the fix should work.
The relevant logic is here: https://searchfox.org/mozilla-central/rev/8cf9cec21122aeab86126ddb566cba52bb6777d8/toolkit/modules/HiddenFrame.jsm#16,23-26

You can start by confirming the bug: Running that logic triggers an error.
Then, after your fix, you need to verify that you are able to iterate over the set.

Comment 10

[Cas Verploegen]

Attachment: Bug 1647894 - Fixed TypeError on HiddenFrame Set. r?robwu

Comment 11

[Cas Verploegen]

I confirmed the bug in the browser console. After changing the code, the bug was indeed gone and it was able to iterate over the gAllHiddenFrames Set without throwing an error. The changes were committed and submitted to Phabricator, with you (Rob Wu) as the reviewer. Thanks a lot for the help, I look forward to your review and working on more bugs in the future.

Comment 12

[Rob Wu [:robwu]]

As you might have noticed, I have reviewed and approved the patch, so it is ready to land. To land the patch, add the checkin-needed keyword in Phabricator, as explained at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Thanks again for your contribution.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b30578a9880
Fixed TypeError on HiddenFrame Set. r=robwu

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4b30578a9880