958735 - 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

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Daniel Holbert [:dholbert]]

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]

Comment 3

[Kartikaya Gupta (email:[e-mail])]

This is still present in latest m-c code.

Comment 4

[shreyas]

Hi,

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

Comment 5

[:Margaret Leibovic]

(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.

Comment 6

[shreyas]

Attachment: function initialization added

Comment 7

[:Margaret Leibovic]

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.

Comment 8

[shreyas]

I tried to apply what the thread on bug-1071877 meant

Comment 9

[Daniel Holbert [:dholbert]]

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.)

Comment 10

[Daniel Holbert [:dholbert]]

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:

Comment 11

[shreyas]

Attachment: bug958735_jswarning4.diff

Comment 12

[:Margaret Leibovic]

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 :)

Comment 13

[shreyas]

Attachment: bug958735_jswarning4.diff

Indentation updated.

Comment 14

[:Margaret Leibovic]

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.

Comment 15

[shreyas]

Attachment: bug958735_jswarning4.diff

I couldn't test it. The reader mode button doesn't seem to appear in my location bar in nightly.

Comment 16

[Daniel Holbert [:dholbert]]

(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/

Comment 17

[shreyas]

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.

Comment 18

[Daniel Holbert [:dholbert]]

(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?)

Comment 19

[shreyas]

No I checked the spelling, even copy pasted it. I had to manually add it.

Comment 20

[Daniel Holbert [:dholbert]]

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.)

Comment 21

[Daniel Holbert [:dholbert]]

(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.)

Comment 22

[shreyas]

I tried running hg pull and hg update and build it. But there's no change in nightly.

Comment 23

[shreyas]

Got it, how should I proceed now?

Comment 24

[Daniel Holbert [:dholbert]]

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.)

Comment 25

[shreyas]

Attachment: bug958735_jswarning4.diff

This one works...

Comment 26

[Daniel Holbert [:dholbert]]

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.)

Comment 27

[shreyas]

Attachment: bug958735_jswarning4.diff

Attachment 8556795 [details] [diff] was the correct one. I am resubmitting the correct patch and making all others obsolete.

Comment 28

[:Margaret Leibovic]

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!

Comment 29

[Carsten Book [:Tomcat]]

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!

Comment 30

[shreyas]

Sorry I didn't get it? Could you please explain? Is there a problem in the patch ? Or the patch file?

Comment 31

[Botond Ballo [:botond]]

(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.

Comment 32

[:Margaret Leibovic]

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.

Comment 33

[shreyas]

Attachment: bug958735_jswarning4.diff

Comment 34

[:Margaret Leibovic]

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

Comment 35

[shreyas]

Attachment: 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")

Comment 36

[shreyas]

Attachment: bug958735_jswarning4.diff

Apparently that patch had some missing lines of code in it. This should work.

Comment 37

[:Margaret Leibovic]

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.

Comment 38

[shreyas]

Made a pull request on  the github repo.

Comment 39

[:Margaret Leibovic]

(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! :)

Comment 40

[:Margaret Leibovic]

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!