JavaScript Warning: "in strict mode code, functions may be declared only at top level or immediately within another function" {file: "chrome://browser/content/Readability.js" line: 436

RESOLVED FIXED

Status

()

Toolkit
Reader Mode
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: dholbert, Assigned: shreyas, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

4 years ago
Noticed this go by in the logcat spew, while running a debug Fennec build today:
{
GeckoConsole(25873): [JavaScript Warning: "in strict mode code, functions may be declared only at top level or immediately within another function" {file: "chrome://browser/content/Readability.js" line: 436 column: 15 source: "      function purgeNode(node) {
E/GeckoConsole(25873): "}]
}

Link to code:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Readability.js?rev=f932b3812c63#436

The issue is that this function is declared inside the scope of a "while" loop, rather than at the top level scope of the enclosing function.

For more, see the first answer here:
http://stackoverflow.com/questions/5758042/which-js-function-declaration-syntax-is-correct-according-to-the-standard

[hg blame says this was introduced by bug 779796 -- adding dependency]
(Reporter)

Updated

4 years ago
Blocks: 880913
Duplicate of this bug: 896520
Duplicate of this bug: 968651
This is still present in latest m-c code.

Updated

3 years ago
Component: Readability → Reader Mode
Product: Firefox for Android → Toolkit

Updated

3 years ago
Mentor: margaret.leibovic@gmail.com
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 4

3 years ago
Hi,

Is this bug still open? I can't find the file Readability.js in my local repository, instead there is a Reader.js.
(Assignee)

Updated

3 years ago
Flags: needinfo?(margaret.leibovic)

Comment 5

3 years ago
(In reply to shreyas from comment #4)
> Hi,
> 
> Is this bug still open? I can't find the file Readability.js in my local
> repository, instead there is a Reader.js.

Yes, this bug is still open. The Readability.js file is located here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/content/Readability.js

This is shared by desktop Firefox and Firefox for Android, so you can make either a desktop build or an Android build to fix this bug.
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 6

3 years ago
Created attachment 8554471 [details] [diff] [review]
function initialization added
Attachment #8554471 - Flags: review?(margaret.leibovic)

Comment 7

3 years ago
Comment on attachment 8554471 [details] [diff] [review]
function initialization added

Review of attachment 8554471 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/reader/content/Readability.js
@@ +432,5 @@
>         * JSDOMParser returns static node lists, not live ones. When we remove
>         * an element from the document, we need to manually remove it - and all
>         * of its children - from the allElements array.
>         */
> +      purgeNode(node);

I'm a bit confused by this change.. could you explain what you're trying to do here?

This warning is about where the purgeNode function is declared, so I don't think calling it elsewhere will fix things.
Attachment #8554471 - Flags: review?(margaret.leibovic) → review-
(Assignee)

Comment 8

3 years ago
I tried to apply what the thread on bug-1071877 meant
(Reporter)

Comment 9

3 years ago
Thanks -- but I don't think bug 1071877 has any relevance to this particular bug here.

(And even if it did -- you're adding a call to a function, which changes behavior. This bug is just about a coding-style issue, which triggers a warning -- so any fix for this should be sure not to change the code's actual behavior.)
(Reporter)

Comment 10

3 years ago
As noted in comment 0, I think the correct fix for this is to move this whole function -- 'function purgeNode(node) { ..etc.. }' -- to be up just before the "while" loop opens, & maybe add a comment just before the function in that new spot, to explain why it's there, saying something like:
  // Helper function, used below in 'while' loop:
(Assignee)

Comment 11

3 years ago
Created attachment 8555059 [details] [diff] [review]
bug958735_jswarning4.diff
Attachment #8555059 - Flags: review?(margaret.leibovic)

Comment 12

3 years ago
Comment on attachment 8555059 [details] [diff] [review]
bug958735_jswarning4.diff

Review of attachment 8555059 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/reader/content/Readability.js
@@ +416,5 @@
> +          purgeNode(node.childNodes[i]);
> +        }
> +        if (node._index !== undefined && allElements[node._index] == node)
> +          delete allelements[node._index];
> +    }

Much better, thanks!

Please just update this patch to use 2-space indentation (we use 2-space indentation in our JS files), and then I'll mark the new patch as r+ and ready for check-in :)
Attachment #8555059 - Flags: review?(margaret.leibovic) → feedback+

Updated

3 years ago
Assignee: nobody → shreyaslakhe

Updated

3 years ago
Attachment #8554471 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
Created attachment 8555187 [details] [diff] [review]
bug958735_jswarning4.diff

Indentation updated.
Attachment #8555187 - Flags: review?(margaret.leibovic)

Comment 14

3 years ago
Comment on attachment 8555187 [details] [diff] [review]
bug958735_jswarning4.diff

Review of attachment 8555187 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/reader/content/Readability.js
@@ +415,5 @@
> +      for (let i = node.childNodes.length; --i >= 0;) {
> +        purgeNode(node.childNodes[i]);
> +      }
> +      if (node._index !== undefined && allElements[node._index] == node)
> +        delete allelements[node._index];

Oops, looks like a typo here - should be allElements (capital E).

Also, looking more closely at the function bode here, I just realized that this won't work, since allElements is a local variable defined within the while loop. To fix this, you can update this purgeNode helper function to also take allElements as a parameter. Sorry for not catching this earlier!

I believe we should hit this _grabArticle function any time that we try to parse an article in reader mode, so you should make sure you test a page in reader mode to make sure there aren't any errors.

If you make a desktop Firefox build, you can test reader mode by flipping "reader.parse-on-load.enabled" to true in about:config to make a reader mode button appear in the toolbar. You could also test on Firefox for Android, where this feature is enabled by default.
Attachment #8555187 - Flags: review?(margaret.leibovic) → review-
(Assignee)

Comment 15

3 years ago
Created attachment 8555385 [details] [diff] [review]
bug958735_jswarning4.diff

I couldn't test it. The reader mode button doesn't seem to appear in my location bar in nightly.
Attachment #8555385 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 16

3 years ago
(In reply to shreyas from comment #15)
> Created attachment 8555385 [details] [diff] [review]
> bug958735_jswarning4.diff
> 
> I couldn't test it. The reader mode button doesn't seem to appear in my
> location bar in nightly.

Did you try flipping the about:config pref that Margaret indicated in comment 14?  I confirmed that I get the icon, if I flip that pref and visit e.g. https://blog.mozilla.org/blog/2015/01/13/firefox-enables-you-to-experience-and-share-more-on-the-web/
(Reporter)

Updated

3 years ago
Flags: needinfo?(shreyaslakhe)
(Assignee)

Comment 17

3 years ago
I had to add the pref in my about:config in nightly. The pref is set to true. I visited https://blog.mozilla.org/blog/2015/01/13/firefox-enables-you-to-experience-and-share-more-on-the-web/ but
couldn't find the reader mode button in my location bar.
Flags: needinfo?(shreyaslakhe)
(Reporter)

Comment 18

3 years ago
(Weird -- the pref (reader.parse-on-load.enabled) already exists in about:config for me -- no need to add it. I can see it by just typing "reader" into the about:config search bar, and then I can double-click it to change it from false to true. Maybe you mistyped when searching for it / adding it?)
(Assignee)

Comment 19

3 years ago
No I checked the spelling, even copy pasted it. I had to manually add it.
(Reporter)

Comment 20

3 years ago
Something must be wrong then... We've got it set it in all.js (the main place where pref defaults is set), and then we override it in firefox.js (to make it default to off).  Each of those should make it show up in about:config.

MXR search, for reference:
http://mxr.mozilla.org/mozilla-central/search?string=reader.parse-on-load.enabled

Perhaps your Nightly is out of date? It looks like these lines in all.js and firefox.js only got added last week, in this push: http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=ad3867772421

(Help|About should give you a datestamp to see your Nightly's datestamp.)
(Reporter)

Comment 21

3 years ago
(Or alternately, if you're using your own local build, that's probably using out-of-date source code & needs updating in order for you to be able to test reader mode on Desktop.)
(Assignee)

Comment 22

3 years ago
I tried running hg pull and hg update and build it. But there's no change in nightly.
(Assignee)

Comment 23

3 years ago
Got it, how should I proceed now?
Flags: needinfo?(dholbert)
(Reporter)

Comment 24

3 years ago
You got the icon? Great! Now you just want to test it & make sure your patch doesn't somehow break it, I suppose. (circling back to comment 14). i.e. poke around at it without your patch, and then poke around at it with your patch, and make sure it behaves the same.

(Margaret may have more concrete suggestions for what to test. I don't imagine you have to be too thorough, since this is unlikely to break stuff.)

Also, I'd say you want to make sure that:
 (a) you can actually see this warning in the Error Console (Ctrl + Shift + J) without your patch, when you visit a page that supports reader mode (like the blog.mozilla.org URL I linked above)
 (b) the warning goes away after you apply your patch & rebuild.

(To see the warning, you'll need to flip the pref "javascript.options.strict" to "true", and you'll have to have "reader.parse-on-load.enabled" = true as well.)
Flags: needinfo?(dholbert)
(Assignee)

Comment 25

3 years ago
Created attachment 8556795 [details] [diff] [review]
bug958735_jswarning4.diff

This one works...
Attachment #8556795 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 26

3 years ago
Comment on attachment 8556795 [details] [diff] [review]
bug958735_jswarning4.diff

(For future reference: when you post an updated patch, you generally want to mark the old one as "obsolete".  There's a checkbox to do that in the "post attachment" page, when you upload the new version; or, you can mark it as obsolete by viewing the "details" page for the old attachment and clicking "edit details".  This will automatically cancel pending review requests on the old version, which is good so that the reviewer knows which version actually needs reviewing.

Anyway, I'll mark the old attachment as obsolete, since I'm already commenting. :) but now you know how to do that, too.)
Attachment #8556795 - Attachment is obsolete: true
Attachment #8556795 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 27

3 years ago
Created attachment 8556977 [details] [diff] [review]
bug958735_jswarning4.diff

Attachment 8556795 [details] [diff] was the correct one. I am resubmitting the correct patch and making all others obsolete.
Attachment #8555059 - Attachment is obsolete: true
Attachment #8555187 - Attachment is obsolete: true
Attachment #8555385 - Attachment is obsolete: true
Attachment #8555385 - Flags: review?(margaret.leibovic)
Attachment #8556977 - Flags: review?(margaret.leibovic)

Comment 28

3 years ago
Comment on attachment 8556977 [details] [diff] [review]
bug958735_jswarning4.diff

Review of attachment 8556977 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Also, thanks dholbert for your mentorship help!
Attachment #8556977 - Flags: review?(margaret.leibovic) → review+

Updated

3 years ago
Keywords: checkin-needed
Hi,

this failed to apply cleanly:

adding 958735 to series file
renamed 958735 -> bug958735_jswarning4.diff
applying bug958735_jswarning4.diff
patching file toolkit/components/reader/content/Readability.js
Hunk #1 FAILED at 404
1 out of 3 hunks FAILED -- saving rejects to file toolkit/components/reader/content/Readability.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug958735_jswarning4.diff

could you take a look (and maybe this need a try run too).

Thanks!
Flags: needinfo?(shreyaslakhe)
Keywords: checkin-needed
(Assignee)

Comment 30

3 years ago
Sorry I didn't get it? Could you please explain? Is there a problem in the patch ? Or the patch file?
Flags: needinfo?(shreyaslakhe) → needinfo?(cbook)
(In reply to shreyas from comment #30)
> Sorry I didn't get it? Could you please explain? Is there a problem in the
> patch ? Or the patch file?

The patch didn't apply cleanly to the latest trunk. You probably need to rebase it locally. You can do this with 'hg pull --rebase', and then post the rebased version.
Flags: needinfo?(cbook)

Comment 32

3 years ago
If you post a rebased version, I can help make a try run for you, or I can just test it locally and land it myself.
(Assignee)

Comment 33

3 years ago
Created attachment 8560839 [details] [diff] [review]
bug958735_jswarning4.diff
Attachment #8556977 - Attachment is obsolete: true
Attachment #8560839 - Flags: review?(margaret.leibovic)

Comment 34

3 years ago
Comment on attachment 8560839 [details] [diff] [review]
bug958735_jswarning4.diff

This still doesn't apply cleanly to mozilla-central or fx-team. Are you sure you updated your tree before rebasing?

Looking at the changelog, the last change was Feb 3:
http://hg.mozilla.org/integration/fx-team/filelog/tip/toolkit/components/reader/content/Readability.js
Attachment #8560839 - Flags: review?(margaret.leibovic) → review-
(Assignee)

Comment 35

3 years ago
Created attachment 8562044 [details] [diff] [review]
bug958735_jswarning4.diff

I had done it "hg pull --rebase", I don't know something must have gone wrong. Submitting the same patch again (after doing "hg pull --rebase")
Attachment #8560839 - Attachment is obsolete: true
Attachment #8562044 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 36

3 years ago
Created attachment 8562138 [details] [diff] [review]
bug958735_jswarning4.diff

Apparently that patch had some missing lines of code in it. This should work.
Attachment #8562044 - Attachment is obsolete: true
Attachment #8562044 - Flags: review?(margaret.leibovic)
Attachment #8562138 - Flags: review?(margaret.leibovic)

Comment 37

3 years ago
Comment on attachment 8562138 [details] [diff] [review]
bug958735_jswarning4.diff

Review of attachment 8562138 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating the patch!

Actually, since you started working on this bug, we've moved Readability.js development over to a github repo here: https://github.com/mozilla/readability

Would you mind formatting your patch as a pull request against that repo, so that we can land it there? There have been some changes to that file since it was lifted out of mozilla-central, and I worry that if we land your patch here, there will be conflicts when we try to update from this shared library.

I know you're just trying to finish a simple bug here, so if this is too complicated, I can also do this for you.
Attachment #8562138 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 38

3 years ago
Made a pull request on  the github repo.
Flags: needinfo?(margaret.leibovic)

Comment 39

3 years ago
(In reply to shreyas from comment #38)
> Made a pull request on  the github repo.

Thanks so much! I just merged your change :)

I filed bug 1134443 to merge this change back into mozilla-central. I'll close this bug as fixed once I land a patch there.

If you're interested, there are lots of other Readability issues being tracked in bug 1102450, and we could definitely use help fixing them! :)
Depends on: 1134443
Flags: needinfo?(margaret.leibovic)

Comment 40

3 years ago
I just landed a patch for bug 1134443, so this will be in mozilla-central soon.

I'm just going to close this out as FIXED, since it landed in the main readability repo.

Thanks so much for your help, and for your patience while we sort out the workflow around this external library. There are plenty more Readability.js bugs if you're interested in helping out with more!
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.