1618563 - Images browser.theme doesn't accept "moz-extension://" url

Status: NEW

(WebExtensions :: Themes, enhancement, P5)

Description

[juhyt80]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:74.0) Gecko/20100101 Firefox/74.0

Steps to reproduce:

When my app calls browser.theme.getCurrent(), it receives theme object with images.theme_frame field set to "moz-extension://8ecf44a8-6e50-4910-bfad-635c4ba11d5a/<path_to_theme_image>".

Actual results:

But if I change some colors and try to reassign theme, I will get an Error: Type error for parameter details (Error processing images.theme_frame: SyntaxError: String "moz-extension://8ecf44a8-6e50-4910-bfad-635c4ba11d5a/<path_to_theme_image>" must be a relative or PNG or JPG data:image URL) for theme.update.

Expected results:

So browser.theme.images.theme_frame doesn't accept "moz-extension://" url, but in my view, it should.

Comment 1

[juhyt80]

Also, there is no optimal and safety way to get this image.

Comment 2

[Rob Wu [:robwu]]

This issue is caused by the fact that we require the URL to either be a data:-URL or a URL relative to the extension, but internally convert the relative URL to an absolute URL when it is set. As a result, the return value of getCurrent() cannot be passed to theme.update() when images from the extension are used.

This is reasonably easy to fix, so I'm marking this as a mentored good-first-bug. Those who are interested in contributing a patch can start by reading https://wiki.mozilla.org/WebExtensions/Contribution_Onramp . Details specific to this bug are as follows:

The definition of the theme.Theme type (i.e. the parameter passed to theme.update and returned by theme.getCurrent) is here: https://searchfox.org/mozilla-central/rev/13b081a62d3f3e3e3120f95564529257b0bf451c/toolkit/components/extensions/schemas/theme.json#85,90,95,99

The ImageDataOrExtensionURL means that the value should be a data-URL or a relative URL.

This issue could be fixed by either ensuring that theme.getCurrent() returns relative URLs,
or by relaxing the type to support absolute moz-extension:-URLs.

If absolute moz-extension:-URLs were to be supported, we also need to account for the fact that a moz-extension:-URLs can point to another extension. At least initially, I would reject moz-extension:-URLs from other extensions until there is demand for the ability to read and modify a theme from another extension. If we do need to support themes from other extensions, then we need to apply logic similar to what we have for the favIconUrl property in the browser.search.get API.

Comment 3

[arjunlalma]

Is anyone working on this? If not, can I be assigned to this?

Comment 4

[Vishal Chaudhary]

I was going through the error and the defaultTheme.details having image.theme_frame was throwing the complete address of the image, so how should i fix this by replacing the "moz-extensions://" by "./" If i am to return relative URLs. This was working in tests and with Theme integrated sidebar. What could be the further approach I should choose because using replace would not be a good option although I did used conditions to check existence of images.theme_frame

Comment 5

[Rob Wu [:robwu]]

(In reply to arjunlalma from comment #3)

Is anyone working on this? If not, can I be assigned to this?

We don't assign bugs until someone has clearly started on a bug (in the form of specific questions or a patch on Phabricator).

(In reply to Vishal Chaudhary from comment #4)

I was going through the error and the defaultTheme.details having image.theme_frame was throwing the complete address of the image, so how should i fix this by replacing the "moz-extensions://" by "./" If i am to return relative URLs. This was working in tests and with Theme integrated sidebar. What could be the further approach I should choose because using replace would not be a good option although I did used conditions to check existence of images.theme_frame

In comment 2 I sketched multiple potential ways to resolve this bug. I can't easily tell from your comment what you're trying to do. Could you attach a patch to Phabricator to start the discussion?
See https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch

Comment 6

[ManishAradwad]

Hii Rob, This is the file I'm supposed to change, right?
https://searchfox.org/mozilla-central/rev/c888ecff616737e27286d99167edafb6805e8494/toolkit/components/extensions/parent/ext-theme.js#440

Comment 7

[Rob Wu [:robwu]]

(In reply to ManishAradwad from comment #6)

Hii Rob, This is the file I'm supposed to change, right?
https://searchfox.org/mozilla-central/rev/c888ecff616737e27286d99167edafb6805e8494/toolkit/components/extensions/parent/ext-theme.js#440

If you're going to solve this by "ensuring that theme.getCurrent() returns relative URLs", then yes.

Comment 8

[ManishAradwad]

Hii Rob,
I've started working on this bug. Currently, my approach is:

1. Use defaultTheme.details to get the theme_frame property from images object
2. Edit this string to get relative url and reassign it to theme_frame(I found this solution for conversion https://stackoverflow.com/questions/6263454/get-relative-url-from-absolute-url)
3. Finally return the defaultTheme.details which will eventually contain the relative URL

Is this approach right?

Comment 9

[Rob Wu [:robwu]]

1. You also need to handle windowOverrides. And theme_frame is not the only property - https://searchfox.org/mozilla-central/rev/4d9cd186767978a99dafe77eb536a9525980e118/toolkit/components/extensions/schemas/theme.json#85,90,95,99
2. You should only strip the prefix if the URL is from the extension itself, i.e. when it starts with extension.baseURL. Absolute URLs from other extensions would then be out of scope for this bug, as I explained in comment 2.

Comment 10

[ManishAradwad]

Can you plz specify what do u mean by handling windowOverrides?

Comment 11

[Rob Wu [:robwu]]

(In reply to ManishAradwad from comment #10)

Can you plz specify what do u mean by handling windowOverrides?

Previously you proposed to return defaultTheme.details with the modified URL, but that is not the only way that the implementation of theme.getCurrent returns. Take a look at the ext-theme.js code that I linked before.

Note: Since you are not supposed to directly modify the .details property of a stored theme, you would naturally already have a separate variable that contains a copy of the details. The most obvious implementation will have one return statement instead of the current two.

Comment 12

[ManishAradwad]

Hii Rob,
As you've suggested,

1. I'm now considering all the properties i.e. theme_frame, additional_backgrounds, headerURL and additionalProperties.
2. I'm creating a new variable tempTheme to handle windowOverrides like follows:

if (windowOverrides.has(windowId)) {
        let tempTheme = windowOverrides.get(windowId).details;
        // Code to return relative URLs 
} else {
        let tempTheme = defaultTheme.details;
        // Same code as above to return relative URLs
}

3. And at the end, a single return statement to return Promise.resolve(tempTheme)
   But as silly as it sounds, I still haven't figured out how to check whether a URL is from same extension or another one. I tried to find ways to see the difference between both URLs but couldn't find it. Could you plz help??

Comment 13

[Rob Wu [:robwu]]

(In reply to ManishAradwad from comment #12)

Hii Rob,
As you've suggested,

1. I'm now considering all the properties i.e. theme_frame, additional_backgrounds, headerURL and additionalProperties.
2. I'm creating a new variable tempTheme to handle windowOverrides like follows:

if (windowOverrides.has(windowId)) {
        let tempTheme = windowOverrides.get(windowId).details;
        // Code to return relative URLs 
} else {
        let tempTheme = defaultTheme.details;
        // Same code as above to return relative URLs
}

There is a way that doesn't require code duplication. Assigning to a variable doesn't result in a copy; To make a shallow copy, consider using the spread syntax in an object literal.

3. And at the end, a single return statement to return Promise.resolve(tempTheme)
   But as silly as it sounds, I still haven't figured out how to check whether a URL is from same extension or another one. I tried to find ways to see the difference between both URLs but couldn't find it. Could you plz help??

What do you need help with? Seeing the value at all? Or telling the difference between them after having seen the values?
You can use the Browser toolbox to debug the browser and see the value when running an actual example. Another way to see values is to use the dump function to dump values to the standard output (if you run Firefox with ./mach run then the value will appear there). Unlike similar methods such as console.log, line endings are not automatically included by dump, so you need to concatenate the parameter for dump with \n).

Comment 14

[juhyt80]

(In reply to ManishAradwad from comment #12)

Hii Rob,
As you've suggested,

1. I'm now considering all the properties i.e. theme_frame, additional_backgrounds, headerURL and additionalProperties.
2. I'm creating a new variable tempTheme to handle windowOverrides like follows:

if (windowOverrides.has(windowId)) {
        let tempTheme = windowOverrides.get(windowId).details;
        // Code to return relative URLs 
} else {
        let tempTheme = defaultTheme.details;
        // Same code as above to return relative URLs
}

3. And at the end, a single return statement to return Promise.resolve(tempTheme)
   But as silly as it sounds, I still haven't figured out how to check whether a URL is from same extension or another one. I tried to find ways to see the difference between both URLs but couldn't find it. Could you plz help??

I think you should also accept the previous URL, cause other extensions may want to use the old image too. (for example, if they want to update just colors and leave image the same)

Comment 15

[Rob Wu [:robwu]]

(In reply to juhyt80 from comment #14)

I think you should also accept the previous URL, cause other extensions may want to use the old image too. (for example, if they want to update just colors and leave image the same)

I agree that it would be useful to also accept other URLs (see comment 2), but that can be done in a follow-up bug (since the initial report here is about themes from the extension itself). Fixing the implementation to also account for other extensions may be a bit too much for an inexperienced contributor.

Comment 16

[ManishAradwad]

I tried seeing the difference between URLs from same extension and one sent by other extension, but both of them are coming out to be similar:
Extension 1 :

function getStyle(themeInfo) 
{ 
  console.log("theme frame temp : " +  themeInfo.images.theme_frame);
  browser.runtime.sendMessage("38366078ab40dc4fe1e783deda680232cb253027@temporary-addon", themeInfo);
  console.log("Message sent");
}
async function getCurrentThemeInfo() 
{
  var themeInfo = await browser.theme.getCurrent();
  getStyle(themeInfo);
}
getCurrentThemeInfo();

Extension 2 :

function handleMessage(message, sender) 
{
  if (sender.id === "7e6b76da58aff81b628565089d00c3e29bf0f001@temporary-addon") {
    console.log("theme frame temp copy : " +  message.images.theme_frame);
    browser.theme.update(message);
  }
}
browser.runtime.onMessageExternal.addListener(handleMessage);

URLs are as follows:
theme frame temp : moz-extension://6c49f82d-7983-4635-adbb-5c3cc83d25c6/1232849758499.jpg
theme frame temp copy : moz-extension://6c49f82d-7983-4635-adbb-5c3cc83d25c6/1232849758499.jpg

Comment 17

[ManishAradwad]

(In reply to ManishAradwad from comment #16)

I tried seeing the difference between URLs from same extension and one sent by other extension, but both of them are coming out to be similar:
Extension 1 :

function getStyle(themeInfo) 
{ 
  console.log("theme frame temp : " +  themeInfo.images.theme_frame);
  browser.runtime.sendMessage("38366078ab40dc4fe1e783deda680232cb253027@temporary-addon", themeInfo);
  console.log("Message sent");
}
async function getCurrentThemeInfo() 
{
  var themeInfo = await browser.theme.getCurrent();
  getStyle(themeInfo);
}
getCurrentThemeInfo();

Extension 2 :

function handleMessage(message, sender) 
{
  if (sender.id === "7e6b76da58aff81b628565089d00c3e29bf0f001@temporary-addon") {
    console.log("theme frame temp copy : " +  message.images.theme_frame);
    browser.theme.update(message);
  }
}
browser.runtime.onMessageExternal.addListener(handleMessage);

URLs are as follows:
theme frame temp : moz-extension://6c49f82d-7983-4635-adbb-5c3cc83d25c6/1232849758499.jpg
theme frame temp copy : moz-extension://6c49f82d-7983-4635-adbb-5c3cc83d25c6/1232849758499.jpg

This is how I should see the difference between URL from same extension and that from different extension, right? Can someone plz point out the mistake here...
Is it necessary to make changes in some properties of theme returned by getCurrent, cause I'm using the returned theme directly.

Comment 18

[Rob Wu [:robwu]]

(In reply to ManishAradwad from comment #17)

(In reply to ManishAradwad from comment #16)

I tried seeing the difference between URLs from same extension and one sent by other extension, but both of them are coming out to be similar:
...
URLs are as follows:
theme frame temp : moz-extension://6c49f82d-7983-4635-adbb-5c3cc83d25c6/1232849758499.jpg
theme frame temp copy : moz-extension://6c49f82d-7983-4635-adbb-5c3cc83d25c6/1232849758499.jpg

This is how I should see the difference between URL from same extension and that from different extension, right? Can someone plz point out the mistake here...

You're comparing the return value of getCurrent() from two different extensions. Unsurprisingly, they are the same, because theme.getCurrent() returns the current theme, which isn't changed by either of your extensions.

I think that there is a misunderstanding of this bug and what's needed to fix it, so let's start again: In comment 12, you described your approach to resolve the bug plus a question on how to check whether the theme is from the current extension. In comment 13, I described how you can debug the browser code (the implementation of getCurrent in ext-theme.js in particular) to inspect relevant variables in the scope, including the ones that I mentioned in comment 9.

When you are able to debug and inspect the variables at runtime, it may become much easier to understand and follow the code. I strongly recommend to try and debug the browser.

Is it necessary to make changes in some properties of theme returned by getCurrent, cause I'm using the returned theme directly.

Yes, but see my last paragraph in comment 11.

Comment 19

[ManishAradwad]

Attachment: Bug 1618563 - Added test_ext_themes.js in xpcshell-common.ini. r?robwu

Comment 20

[ManishAradwad]

Attachment: Bug 1618563 - Fix absolute url issue of getCurrent. r?robwu

Comment 21

[Caitlin Neiman [:caitmuenster]]

Hey Manish! It's been a few weeks and we wanted to check in. Can you attach the test file for this patch?

Comment 22

[Caitlin Neiman [:caitmuenster]]

Hi Manish! Since we haven't heard from you in awhile, we're going to unassign you from this bug and open it up to other contributors.. If you'd like to pick it back up, please feel free to do so!

Comment 23

[Sonia Singla :soniasingla]

Hi Caitlin! Can I take this bug?

Regards
Sonia

Comment 24

[Rob Wu [:robwu]]

Hi Sonia, sure! If you still have questions despite reading the existing comments on this bug, feel free to ask.

Comment 25

[Sonia Singla :soniasingla]

Attachment: Bug 1618563 - Fixed moz-extension:// URL for browser.theme images. r=robwu

Comment 26

[Caitlin Neiman [:caitmuenster]]

Hey Sonia, how is it going with this bug? Is there anything we can do to help you move it forward?

Comment 27

[Caitlin Neiman [:caitmuenster]]

Hey Sonia, since we haven't heard from you in awhile we are going to unassign you from this bug and open it up to other contributors.

If you would like to continue working on it, please drop a comment!

Comment 28

[Ikeh Chukwuka Favour]

Hello Caitlin, I would love to fix the bug. I'm an Outreachy applicant.

Comment 29

[Ikeh Chukwuka Favour]

Hello Rob Wu I would love to fix the bug. I'm an Outreachy applicant.

Comment 30

[Rob Wu [:robwu]]

Hi Ikeh, first step is to set up a development environment. See comment 2 for more context on this bug.
There was a previous (incomplete/incorrect) attempt to fix this bug. Although incomplete/incorrect, you may still be able to learn a bit by reading the discussion.

If you are completely unfamiliar with browser extensions, I recommend to familiarize yourself with the topic, and specifically the theme API of this bug.

Comment 31

[Ikeh Chukwuka Favour]

Thank you Rob. Honestly this is my first time working with browser extensions, even though I've used them and always wanted to build some also. Already taking a course on that.

Comment 32

[Ikeh Chukwuka Favour]

Hello Rob, please how do I locate the bug? What includes the bug or should I make my patch to?
I've successfully set up a development environment. Just asking what next should I do to contribute to the project?

Comment 33

[Rob Wu [:robwu]]

I have answered the questions in Matrix. If I did not, please ask again (but be more specific).

Comment 34

[Ikeh Chukwuka Favour]

Attachment: ext-theme.js: Images browser.theme doesn't accept "moz-extension://" url

code patch to ext-theme.js theme.getCurrent method.
The patch is set convert an absolute URL to relative URl. First checks if the theme object properties contains value with absolute URL, then returnung a relative path to it.

Comment 35

[Ikeh Chukwuka Favour]

Hello Rob, in comment 9 you made mention of the extension.baseURL as the prefix added when theme.getCurrent() is invoked, right? (Hope I'm right.) I did search for the extension.baseURL, but found none. I did find baseURI and also extension. But want to ask if the baseURI is the same with extension.baseURL with emphasis on the baseURL property?

Comment 36

[Rob Wu [:robwu]]

(In reply to Ikeh Chukwuka Favour from comment #35)

Hello Rob, in comment 9 you made mention of the extension.baseURL as the prefix added when theme.getCurrent() is invoked, right? (Hope I'm right.) I did search for the extension.baseURL, but found none.

Where did you look? When I search with https://searchfox.org/mozilla-central/search?q=baseURL&path=extensions%2F&case=true&regexp=false I see the definition of baseURL in Extension.jsm

Comment 37

[Ikeh Chukwuka Favour]

(In reply to Rob Wu [:robwu] from comment #36)

(In reply to Ikeh Chukwuka Favour from comment #35)

Hello Rob, in comment 9 you made mention of the extension.baseURL as the prefix added when theme.getCurrent() is invoked, right? (Hope I'm right.) I did search for the extension.baseURL, but found none.

Where did you look? When I search with https://searchfox.org/mozilla-central/search?q=baseURL&path=extensions%2F&case=true&regexp=false I see the definition of baseURL in Extension.jsm

Ohh, I did the search on the ext-theme.js file.
Is it safe to ask, "if the baseURI in the ext-theme.js is the same with extension.baseURL with emphasis on the baseURL property"?

Comment 38

[Rob Wu [:robwu]]

(In reply to Ikeh Chukwuka Favour from comment #37)

Is it safe to ask, "if the baseURI in the ext-theme.js is the same with extension.baseURL with emphasis on the baseURL property"?

They are different types. One is a nsIURI instance, the other is a string.
I suggest that you use the Browser Toolbox to inspect the values. Then you will be more effective at debugging because then don't need to ask me any more.

After opening the browser toolbox (see https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox), you can use Ctrl-P, ext-theme.js to open the ext-theme.js file. Put a breakpoint in the function that you're modifying, and call the extension API from a test extension to trigger the breakpoint.

Comment 39

[Ikeh Chukwuka Favour]

Attachment: Bug 1618563 - removed the setURL function which had a lot of code duplications. Redefined the absoluteaToRelativeURL function to be more precised. r=robwu

Comment 40

[Ikeh Chukwuka Favour]

Attachment: Bug 1618563 - Made changes to the ext-theme.js file: changed the Object.getOwnPropertyNames method to Object.keys method as recommended. r=robwu

Depends on D93896

Comment 41

[Ikeh Chukwuka Favour]

Attachment: Bug 1618563 - merged two chnagesets,618243 and 618241 together. r=robwu

Depends on D93896

Comment 42

[Ikeh Chukwuka Favour]

Attachment: Bug 1618563 - Cloned the theme properties, and also the theme.images property. Added unit test for the patch submitted. r=robwu.

Comment 43

[juhyt80]

Well, thanks for work guys, I hope this bug will be fixed finally! Waiting for 9 months already :)

Comment 44

[Ikeh Chukwuka Favour]

Attachment: Bug 1618563 - Cloned the theme properties, and also the theme.images property. Added unit test for the patch submitted. r=robwu

Comment 45

[Ikeh Chukwuka Favour]

Attachment: Bug 1618563 - Cloned the theme properties, and also the theme.images property. Added unit test for the patch submitted. r=robwu

Comment 46

[Ikeh Chukwuka Favour]

Attachment: Bug 1618563 - Cloned the theme properties, and also the theme.images property. Added unit test for the patch submitted. r=robwu. Differential Revision: https://phabricator.services.mozilla.com/D95416

Comment 47

[Caitlin Neiman [:caitmuenster]]

Hey Ikeh, since we haven't heard from you in awhile, we're going to open this bug up for other contributors. If you'd like to come back to this, let us know and we'll reassign you!

Comment 48

[Rob Wu [:robwu]]

To clean up the list of linked Phabricator revisions, I commandeered ten of the revisions and marked them as abandoned.

I left the last one with pending review feedback open in case anyone is interested in the past feedback.

Comment 49

[juhyt80]

Hi, as this issue is holding for a year now without a fix, I would like to contribute a patch myself. I can see there is some code in phabricator that didn't pass the review.
I din't do any contributions to FF, and I never worked with tools you use, but I would love to learn.

Comment 50

[Rob Wu [:robwu]]

(In reply to juhyt80 from comment #49)

Hi, as this issue is holding for a year now without a fix, I would like to contribute a patch myself. I can see there is some code in phabricator that didn't pass the review.
I din't do any contributions to FF, and I never worked with tools you use, but I would love to learn.

Welcome!

Working on a bug is easier if you have a good understanding of the bug and area. Are you familiar with extensions and/or JavaScript?

Take a look at the contribution guide to get started ( https://wiki.mozilla.org/WebExtensions/Contribution_Onramp ), and feel free to ask questions.

Comment 51

[juhyt80]

(In reply to Rob Wu [:robwu] from comment #50)

(In reply to juhyt80 from comment #49)

Hi, as this issue is holding for a year now without a fix, I would like to contribute a patch myself. I can see there is some code in phabricator that didn't pass the review.
I din't do any contributions to FF, and I never worked with tools you use, but I would love to learn.

Welcome!

Working on a bug is easier if you have a good understanding of the bug and area. Are you familiar with extensions and/or JavaScript?

Take a look at the contribution guide to get started ( https://wiki.mozilla.org/WebExtensions/Contribution_Onramp ), and feel free to ask questions.

I am frontend develper (sort of), I am making some addons and I have quiet a few popular themes on AMO.
So, I know JS, aware of bug, as I am a reporter, and I hope I am able to make a patch. I can see the files I need to change, so I will try to prepare patch and post it on phabricator shortly. Thanks

Comment 52

[juhyt80]

I would assume, that the easiest fix and perhaps the most safe one, is to convert "moz-extension" urls of other addons (in theme.getCurrent() method) to the data URL, like Rob Wu suggested, made in here https://searchfox.org/mozilla-central/rev/13b081a62d3f3e3e3120f95564529257b0bf451c/browser/components/extensions/parent/ext-search.js#58-69

Comment 53

[Rob Wu [:robwu]]

(In reply to juhyt80 from comment #52)

I would assume, that the easiest fix and perhaps the most safe one, is to convert "moz-extension" urls of other addons (in theme.getCurrent() method) to the data URL, like Rob Wu suggested, made in here https://searchfox.org/mozilla-central/rev/13b081a62d3f3e3e3120f95564529257b0bf451c/browser/components/extensions/parent/ext-search.js#58-69

That would work for many cases, but not all, because only PNG/JPG are supported according to https://searchfox.org/mozilla-central/rev/fc95c6ad297d9d257f05599d01741503f3f57326/toolkit/components/extensions/Schemas.jsm#1117-1118 (referenced via https://searchfox.org/mozilla-central/rev/fc95c6ad297d9d257f05599d01741503f3f57326/toolkit/components/extensions/schemas/theme.json#79,85,90,95,99).

There is an open bug for SVG (at bug 1491790), but there are also other image types.

Users of the search extension API are expected to render the image in web_accessible_resources, hence the need to convert to a data:-URL. In the case of themes, extensions themselves are not likely to need actual access to the image. Therefore it may be more useful to just accept absolute moz-extension:-URLs, even if they're from other extensions.

Comment 54

[juhyt80]

Okay, so if we are gonna accept absolute mox-extension: urls

1. Somehow we need to relax input format of ImageDataOrExtensionURL. It is using imageDataOrStrictRelativeUrl, and we need to came up with some other format, that will accept absolute paths only within mox-extension: scheme, that can be a problem.
2. Also, need to do something with, baseURI.resolve(url) which is defined, as I understood here and "resolves a relative string into an absolute".

(In reply to Rob Wu [:robwu] from comment #53)

That would work for many cases, but not all, because only PNG/JPG are supported according to https://searchfox.org/mozilla-central/rev/fc95c6ad297d9d257f05599d01741503f3f57326/toolkit/components/extensions/Schemas.jsm#1117-1118 (referenced via https://searchfox.org/mozilla-central/rev/fc95c6ad297d9d257f05599d01741503f3f57326/toolkit/components/extensions/schemas/theme.json#79,85,90,95,99).

There is an open bug for SVG (at bug 1491790), but there are also other image types.

Users of the search extension API are expected to render the image in web_accessible_resources, hence the need to convert to a data:-URL. In the case of themes, extensions themselves are not likely to need actual access to the image. Therefore it may be more useful to just accept absolute moz-extension:-URLs, even if they're from other extensions.