1906552 - Fakespot addon gets triggered needlessly, and spends a ton of time in non-ion path (Blinterp)

Status: RESOLVED FIXED

(WebExtensions :: Developer Outreach, defect, P5)

Description

[Mayank Bansal]

1. Add fakespot extension to Firefox Nightly desktop browser
2. Go to https://prettier.io/playground
   From the left-pane, select the following :
   parser = Babel (should be default)
   Debug = "Show input" only. (UNCHECK "Show AST")
3. Paste the attached sample text into the input pane

AR:https://share.firefox.dev/3zwFjWR

ER:

1. The extension shouldnt trigger for this action
2. Even when triggered, the extension should take the fast ionmonkey path inside the Javascript engine. So it appears that the JS code inside the extension is using some pattern that ionmonkey cannot handle.

Comment 1

[Mayank Bansal]

Attachment: about:support

Comment 2

[Mayank Bansal]

Attachment: Sample text file (source code of d3.js)

Comment 3

[Mayank Bansal]

(i have ccd some folks who may have an opinion, or would know enough to add the relevant people)

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Rob is away this week and so I took a quick look into this issue.

Based on what I can spot in the profiler data linked to comment 0, it looks that the Fakespot extensions is handling tabs.onUpdated events, and interestingly when I did try the STR from comment 0 I noticed the following error being logged in the console:

InternalError: too much recursion 2 background.js:1:137089

I'm not 100% sure if the "too much recursion" may be part of what prevents ionmonkey path to handle that code, but I wouldn't exclude that it may related neither.

I took a brief look to the extension background page to see if I could gather some more details (but I looked to the minified code for the background page and so this may not be 100% accurate) and I got the impression that when the "too much recursion" InternalError is being hit the tabs.onUpdated listener is checking if the tab url (which in the STR from comment 0 is pretty big, because it is derived from the long js code string pasted into the prettier playground page) belongs to one of the sites that are handled by the extension (with the purpose of changing the color of the browser action icon based on the outcome of those checks to signal that to the users).

In particular it seems that the listener may be checking for the "homeDepot" urls when it hits the "too much recursion" error (but as mentioned earlier that was based on the minified code and so would be better to confirm that on the non-minified code through the related sourcemaps).

Comment 5

[:Gijs (he/him)]

might want to file a separate js engine bug if we think there are engine improvements - sdetar can potentially help there. But for now, we don't track Fakespot issues inside the Firefox product in bugzilla (only issues with the builtin Review Checker support). So moving to webextension outreach.

Comment 6

[Abhishek Nagekar [:Abhi]]

For the developer outreach (if intended), could you drop a comment following the outreach request template. Thank you!

Comment 7

[Abhishek Nagekar [:Abhi]]

We're using a different outreach approach here. As a result, assigning the issue to Will.

Tom, no further action needed here.

Comment 8

[William Durand [:willdurand]]

Indeed, I sent this bug to the Fakespot add-on (for desktop) developers. They are aware of it. I am going to keep this bug open in case they want to add or ask more information.