Closed Bug 1395898 Opened 7 years ago Closed 7 years ago

[OOP] jar cache incorrectly handled non-existent file paths

Categories

(WebExtensions :: Request Handling, defect)

defect
Not set
normal

Tracking

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 fixed, firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- verified

People

(Reporter: vtamas, Assigned: haik)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

Attached image 2017-09-01_1345.png
[Affected versions]:
Firefox 57.0a1 (2017-08-31)
Firefox 56.0b8 (20170831165232)

[Affected platforms]:
Windows 10 64-bit
Mac OS X 10.12.3

[Steps to reproduce]:
1.Launch Firefox with a clean profile.
2.Make sure that  "extensions.webextensions.remote” pref is set to true.
3.Install the following webextension: https://addons.mozilla.org/ro/firefox/addon/screenshot-capture-annotate/?src=ss
4.Navigate to a new webpage (e.g. https://www.amazon.com/)
5.Click on the webextension icon from toolbar.
6.Select any option from the list.


[Expected Results]:
The webextension options work without any issue.


[Actual Results]:
- Capture tabs are never loaded.
- This issue is not reproducible with "extensions.webextensions.remote” pref is set to false.
- The following error appears in Browser Console: 
Unchecked lastError value: Error: Could not establish connection. Receiving end does not exist. ExtensionCommon.jsm:407
- See attached screenshot.

[Regression Range]:
Last good revision: 29a2d717c525f7f50ed5d7b7afe2aa25a96a6bbd
First bad revision: 9878f96c556e62c4253c2fec24e156309a29a1ac
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=29a2d717c525f7f50ed5d7b7afe2aa25a96a6bbd&tochange=9878f96c556e62c4253c2fec24e156309a29a1ac

Looks like the following bug has the  changes which introduced the regression: Bug 1390346


[Additional notes]:
The extension is not uninstalled after clicking on “Remove” button from about:addons.
@haik: Not much to go on here yet, but certainly seems to be jar caching based on the testing i did.  Any thoughts?
Flags: needinfo?(haftandilian)
(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> @haik: Not much to go on here yet, but certainly seems to be jar caching
> based on the testing i did.  Any thoughts?

I was looking at this yesterday and couldn't make sense of it, but I just mis-typed a moz-extension:// URI and realized the failure for a non-existent file is the same as what is happening here.

The problem is that the code I added to use SimpleChannel for remote moz-extension URI's that resolve to a cached JAR has a bug (a silly careless mistake) in the error handling: the return value from AsyncOpen2() is being ignored.

That breaks this particular extension because the extension's edit.html file includes a script ("javascripts/libs/jquery.ui.resizable.min.js") that is missing from the XPI. Removing the offending <script ...> line from edit.html makes things work again. That's

  <script type="text/javascript" src="javascripts/libs/jquery.ui.resizable.min.js"></script>

The issue with the fix for bug 1390346 is that the return value from "mJarChannel->AsyncOpen2(mListener)" for the cache hit case in ExtensionStreamGetter::GetAsync() is being ignored. Ignoring the AsyncOpen2() failure is causing the load to hang.
Flags: needinfo?(haftandilian)
Assignee: nobody → haftandilian
Thanks Haik!
Comment on attachment 8904040 [details]
Bug 1395898 - Awesome Screenshot webextension does not work.

https://reviewboard.mozilla.org/r/175768/#review180818
Attachment #8904040 - Flags: review?(jmathies) → review+
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e12897c25fc5
Awesome Screenshot webextension does not work. r=jimm
https://hg.mozilla.org/mozilla-central/rev/e12897c25fc5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Haik, can you please request uplift to 56 when you get a chance?

Andreas, can you contact the Awesome Screenshots devs about their attempts to load a nonexistent script?

Thanks.
Flags: needinfo?(haftandilian)
Flags: needinfo?(awagner)
Comment on attachment 8904040 [details]
Bug 1395898 - Awesome Screenshot webextension does not work.

Approval Request Comment
[Feature/Bug causing the regression]:
The fix for bug 1390346 introduced this regression.

[User impact if declined]:
Some extension pages may fail to load--they'll be stuck "loading" and never complete.

[Is this code covered by automated tests?]:
This particular failure is not, but the area being changed is well covered.

[Has the fix been verified in Nightly?]:
I've verified this is fixed in today's (2017-09-05) Nightly.

[Needs manual test from QE? If yes, steps to reproduce]: 
QE should verify the Beta fix. Use the current version 3.0.21 of the Awesome Screenshot extension from the description (newer versions might get a fix that avoids the problem.)

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
This is a small followup fix to add a missing check of a return value. The change is similar to what is already used in ExtensionProtocolHandler.cpp for other cases.

[String changes made/needed]:
None
Flags: needinfo?(haftandilian)
Attachment #8904040 - Flags: approval-mozilla-beta?
Hi Vasilica,
Can you help check if this issue was fixed in the latest nightly?
Flags: needinfo?(vasilica.mihasca)
Flags: qe-verify+
The author has been notified.
Flags: needinfo?(awagner)
Depends on: 1397257
Attached image awesomescreenshot.gif
Verified on Firefox 57.0a1 (20170905220108) under Windows 10 64-bit and Mac OS X 10.12.3 and the webextensions pages are successfully loaded but the following errors are still displayed in browser console:
    Unchecked lastError value: Error: Could not establish connection. Receiving end does not 
    exist.  ExtensionCommon.jsm:407
    Error: Response handle went out of scope  undefined
    Unchecked lastError value: Error: Response handle went out of scope  ExtensionCommon.jsm:407
    this.widget is null  ext-browserAction.js:523

Any thoughts about these errors?

Also a new regression was encountered on Windows when removing the Awesome Screenshot webextensions for a second time: Bug 1397257
Flags: needinfo?(vasilica.mihasca)
Flags: needinfo?(haftandilian)
I'm not familiar with the JS side of extension loading. Shane, is that something you could take a look at?
Flags: needinfo?(haftandilian) → needinfo?(mixedpuppy)
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #13)
> Created attachment 8905033 [details]
> awesomescreenshot.gif
> 
> Verified on Firefox 57.0a1 (20170905220108) under Windows 10 64-bit and Mac
> OS X 10.12.3 and the webextensions pages are successfully loaded but the
> following errors are still displayed in browser console:
>     Unchecked lastError value: Error: Could not establish connection.
> Receiving end does not 
>     exist.  ExtensionCommon.jsm:407
>     Error: Response handle went out of scope  undefined
>     Unchecked lastError value: Error: Response handle went out of scope 
> ExtensionCommon.jsm:407
>     this.widget is null  ext-browserAction.js:523
> 
> Any thoughts about these errors?

These may or may not be related and I'm betting not since these have nothing to do with file access.  Lets have another bug on that.

> Also a new regression was encountered on Windows when removing the Awesome
> Screenshot webextensions for a second time: Bug 1397257

Is this regression due to the patch in this bug?  If not please don't block this bug on it.
Summary: Awesome Screenshot webextension does not work → [OOP] jar cache incorrectly handled non-existent file paths
(In reply to Haik Aftandilian [:haik] from comment #14)
> I'm not familiar with the JS side of extension loading. Shane, is that
> something you could take a look at?

I ni? aswan on the bug created for that issue.
Flags: needinfo?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> These may or may not be related and I'm betting not since these have nothing
> to do with file access.  Lets have another bug on that.

Filed Bug 1397730

> Is this regression due to the patch in this bug?  If not please don't block
> this bug on it.

Yes. Bug 1397257 was regressed by this one.

Based on Comment 13 and Comment 15 I am marking this bug as Verified fixed on Firefox 57.
Status: RESOLVED → VERIFIED
Comment on attachment 8904040 [details]
Bug 1395898 - Awesome Screenshot webextension does not work.

Fix a regression and was verified. Beta56+.
Attachment #8904040 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I'd like to hold off on uplifting this to beta until bug 1397257 is fixed.
Comment on attachment 8904040 [details]
Bug 1395898 - Awesome Screenshot webextension does not work.

Per comment #19, hold off on uplifting to Beta56.
Attachment #8904040 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
See Also: → 1398908
Now that bug 1397257 is fixed, I'd like to re-request we uplift this to Beta. Uplift of bug 1397257 is also required for this.
Flags: needinfo?(gchang)
Comment on attachment 8904040 [details]
Bug 1395898 - Awesome Screenshot webextension does not work.

Fix for a regression from 56 that may prevent some extensions from loading.
This should uplift along with the fix from bug 1397257.
Attachment #8904040 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Flags: needinfo?(gchang)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.