1455402 - Stop calculating the size attribute for extensions

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[YUKI "Piro" Hiroshi]

When the directory contains any symbolic link created on WSL environment, Firefox cannot load an addon as a temporary addon, from about:debugging. For example, my addon "Tree Style Tab" uses eslint as a linter and there is a symbolic link "node_modules/.bin/eslint" created by "npm install" executed on Ubuntu on WSL. Firefox tries to read the file, but it is a symbolic link to "node_Modules/eslint/bin/eslint.js" and the link is unable to read on Windows side. As the result, Firefox cannot load the addon as a temporary addon anymore.


Steps to reproduce:

1. Boot Windows 10 (Fall Creators Update or later.)
2. Install "Ubuntu" from the app store and initialize.
3. Start Ubuntu on WSL.
4. Install dependencies: "sudo apt install git nodejs"
5. Go to an Windows directory: "cd /mnt/c/Users/(username)/"
6. Make a new directory: "mkdir test"
7. Go to the directory: "cd test"
8. Create "manifest.json" with minimum content.
9. Put a symbolic link there: "ln -s manifest.json ./symlink"
10. Start Firefox.
11. Go to about:debugging.
12. Hit Ctrl-Shift-J to open the Browser Console.
12. Click "Load Temporary Add-on" button and
    choose the manifest.json created at the step 8.


Actual result:

The addon is not loaded and there is an error report in the browser console:
----
Object { operation: "open", path: "C:\\...\\symlink", winLastError: 1920, stack: "", fileName: "(unknown module)", lineNumber: undefined } Controls.js:85:9
----


Expected result:

The addon is loaded and there is no error in the browser console.


Result of mozregression:

Last good:
build_file: C:\Users\piro\.mozilla\mozregression\persist\19d7d934850c--mozilla-inbound--target.zip
build_type: inbound
build_url: https://queue.taskcluster.net/v1/task/RYN2kKLsQCWAn6UPiSmLYg/runs/0/artifacts/public%2Fbuild%2Ftarget.zip
changeset: 19d7d934850c1c3efcda7b1227cfa5ffcd2ce63b
pushlog_url: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=19d7d934850c1c3efcda7b1227cfa5ffcd2ce63b&tochange=bc804d75911a04cc104898a0cee538e761f5582b
repo_name: mozilla-inbound
repo_url: https://hg.mozilla.org/integration/mozilla-inbound
task_id: RYN2kKLsQCWAn6UPiSmLYg

First bad:
build_file: C:\Users\piro\.mozilla\mozregression\persist\bc804d75911a--mozilla-inbound--target.zip
build_type: inbound
build_url: https://queue.taskcluster.net/v1/task/Fl3C4rrbRZuknJc9kITqdg/runs/0/artifacts/public%2Fbuild%2Ftarget.zip
changeset: bc804d75911a04cc104898a0cee538e761f5582b
pushlog_url: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=09c70b07e2e4f6df6b7ad750e8d46a00209648fb&tochange=bc804d75911a04cc104898a0cee538e761f5582b
repo_name: mozilla-inbound
repo_url: https://hg.mozilla.org/integration/mozilla-inbound
task_id: Fl3C4rrbRZuknJc9kITqdg

So the commit https://hg.mozilla.org/integration/mozilla-inbound/rev/bc804d75911a seems to cause this regression.

Comment 1

[YUKI "Piro" Hiroshi]

Oops, sorry the step 4 is not required. I forgot to remove the step.

Comment 2

[Andrew Swan [:aswan]]

I haven't reproduced this but from inspection, it looks like this is failing while computing the "size" property of the extension.  The only thing that cares about that property is tests, lets fix this by just removing that property (and associated tests of course)

Comment 3

[Masatoshi Kimura [:emk]]

OS.File.stat will fail if the file is a WSL symlink:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc804d75911a#l1.184

Since OS.File.stat can fail for various reasons (no permissions, sharing violation, etc.), iterFiles should handle exceptions gracefully.

Comment 4

[YUKI "Piro" Hiroshi]

Attachment: Add handling of file permission errors

Still there is no automated test. How should I do that?

Comment 5

[YUKI "Piro" Hiroshi]

Plan A: If WSL is activated on the Mozilla's automated test environment for Windows 10, we can create a symbolic file like:

```
c:\windows\system32\wsl.exe bash -c "touch /tmp/file; ln -s /tmp/file /mnt/c/Users/Public/tempaddon/file"
```

and we can use it from a test. Otherwise, Plan B: we need to replace "OS.File.stat()" itself to a version which raises an error. Plan A is robust but hard, Plan B is easy.

Comment 6

[YUKI "Piro" Hiroshi]

Attachment: Add graceful error handling about inaccessible files

I've implemented automated test based on stabbing of OS.File.stat(). How about this?

Comment 7

[Andrew Swan [:aswan]]

Wait, what about the proposal from comment 2?
I'm open to doing something like this if there's an issue with the approach from comment 2, but for starters, we should not be adding a new test-only error code.

Comment 8

[YUKI "Piro" Hiroshi]

(In reply to Andrew Swan [:aswan] from comment #7)
Sorry, I misunderstood your comment like as: this bug won't be fixed at lower layer implementation. And because this is very serious for me (this completely blocks addon development on Firefox 61 and later for me), I tried to write something patch...

Comment 9

[Kris Maglione [:kmag]]

There's no need to add additional error checking if we remove the size calculation, which we want to do anyway, since it isn't used outside of tests.

Comment 10

[YUKI "Piro" Hiroshi]

Attachment: don't check size of temporary webextension

Bug 1455402: don't check size of temporary webextension

Comment 11

[YUKI "Piro" Hiroshi]

Comment on attachment 8976076 [details] [diff] [review]
don't check size of temporary webextension

(In reply to Andrew Swan [:aswan] from comment #2)
> I haven't reproduced this but from inspection, it looks like this is failing
> while computing the "size" property of the extension.  The only thing that
> cares about that property is tests, lets fix this by just removing that
> property (and associated tests of course)

Codes looks touching to "size" of addons are:
https://dxr.mozilla.org/mozilla-central/search?q=ddon.size&=mozilla-central
The property is used on automated test. I think I should not simply remove the "size" property, so, instead I've added a code to skip size calculation for temporary addons. How about this?

Comment 12

[Kris Maglione [:kmag]]

(In reply to YUKI "Piro" Hiroshi from comment #11)
> Codes looks touching to "size" of addons are:
> https://dxr.mozilla.org/mozilla-central/search?q=ddon.size&=mozilla-central
> The property is used on automated test. I think I should not simply remove
> the "size" property, so, instead I've added a code to skip size calculation
> for temporary addons. How about this?

The size property is only used in tests to test the size property. The use in extensions.js is not important. We want to remove the property altogether.

Comment 13

[YUKI "Piro" Hiroshi]

Comment on attachment 8976076 [details] [diff] [review]
don't check size of temporary webextension

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #12)
> (In reply to YUKI "Piro" Hiroshi from comment #11)
> The size property is only used in tests to test the size property. The use
> in extensions.js is not important. We want to remove the property altogether.

OK, I try to do that.

Comment 14

[YUKI "Piro" Hiroshi]

Attachment: remove obsolete code to stat file

Bug 1455402: remove "size" attribute from addon

Comment 15

[YUKI "Piro" Hiroshi]

Attachment: remove size attribute from addon

Comment 16

[YUKI "Piro" Hiroshi]

Test result of previous version patch at tryserver:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a72a69db93d502d357d2dff7add45c185dbe02ec&selectedJob=178916328
The last ESlint warning has been fixed at the latest version patch.
Other three errors are unrelated to this change.

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8976426 [details] [diff] [review]
remove size attribute from addon

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

Thanks

Comment 18

[Natalia Csoregi [:nataliaCs]]

Patch failed to apply.

Hunk #1 FAILED at 115
1 out of 2 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/internal/XPIDatabase.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh base-0f9781d5d011

Comment 19

[YUKI "Piro" Hiroshi]

Attachment: remove size attribute from addon

Comment 20

[YUKI "Piro" Hiroshi]

Comment on attachment 8979042 [details] [diff] [review]
remove size attribute from addon

The r+ status is carried over from previous patch.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3442210b7b8
Remove size attribute from addon. r=kmag

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/d3442210b7b8

Comment 23

[YUKI "Piro" Hiroshi]

After I updated my Windows 10 to "April 2018 Update" and recreated symbolic links, this problem is gone without the patch, because "ln -s" on April 2018 Update produces genuine symbolic link on NTFS correctly. The error I saw seems to be triggered by symbolic links created by old version WSL. So, the exact "platform" maybe "Windows 10 Creators Update" or "Windows 10 Fall Creators Update".

Anyway, something recent change actually introduces fragility for such unexpected errors, and I think the patch should be uplifted to beta.

Comment 24

[Kris Maglione [:kmag]]

Feel free to request uplift. It's up to release managers to decide whether to take it. I don't have any particular objection, though. Deleting unnecessary code is pretty low-risk.

Comment 25

[YUKI "Piro" Hiroshi]

Comment on attachment 8979042 [details] [diff] [review]
remove size attribute from addon

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1452827
[User impact if declined]: Impossible to load temporary addon on Windows 10 Fall Creators Update or older versions.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The change just removes only obsolete codes and existing tests are still green.
[String changes made/needed]: None.

Comment 26

[Kris Maglione [:kmag]]

(In reply to YUKI "Piro" Hiroshi from comment #25)
> [User impact if declined]: Impossible to load temporary addon on Windows 10
> Fall Creators Update or older versions.

*Unpacked temporary add-ons whose directories contain WSL symlinks.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This seems really edge-case to me. While it's mostly just code deletion, this still doesn't feel like a work flow that we're likely to see much in the wild, so I think we can let this ride the trains.

Comment 28

[Kris Maglione [:kmag]]

It might be worth noting that this also apparently causes problems for unpacked extensions with node_modules directories, where we wind up having to stat thousands of files, with a round trip to the IO thread for each.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Any idea how common that is in practice? Without knowing that, it's hard to judge how relevant that is.

Comment 30

[Kris Maglione [:kmag]]

No idea. I suspect fairly common, but I have no data to back that up. See bug 1462669.

Although, to be clear, this is not exactly a new problem in 61, just an old problem with a different shape. In older versions, we still statted all of the files, but we did it on the main thread. So it probably happened faster, but blocked the main thread in the process. Now, it takes longer to install the extension, but doesn't block the main thread.