1384389 - Test browser_all_files_referenced.js should extract URIs for all registered resource packages from libxul binary

Status: RESOLVED FIXED

(Toolkit :: General, enhancement, P3)

Description

[Chung-Sheng Fu [:cfu]]

The test browser/base/content/test/static/browser_all_files_referenced.js extracts constant strings from libxul binary.
But it only regards strings as real chrome or resource URIs by the following conditions:

> if (/chrome:\/\/[a-zA-Z09 -]+\/(content|skin|locale)\//.test(string) ||
>     /resource:\/\/gre.*\.[a-z]+/.test(string))

http://searchfox.org/mozilla-central/source/browser/base/content/test/static/browser_all_files_referenced.js#493

It had better accept URIs for all resource packages registered in manifests.

Comment 1

[Florian Quèze [:florian]]

(In reply to Chung-Sheng Fu [:cfu] from comment #0)
> The test browser/base/content/test/static/browser_all_files_referenced.js
> extracts constant strings from libxul binary.
> But it only regards strings as real chrome or resource URIs by the following
> conditions:
> 
> > if (/chrome:\/\/[a-zA-Z09 -]+\/(content|skin|locale)\//.test(string) ||
> >     /resource:\/\/gre.*\.[a-z]+/.test(string))
> 
> http://searchfox.org/mozilla-central/source/browser/base/content/test/static/
> browser_all_files_referenced.js#493
> 
> It had better accept URIs for all resource packages registered in manifests.

Do you have examples of valid resource packages that aren't caught by these rules?

Comment 2

[Chung-Sheng Fu [:cfu]]

Since bug 863246, we have a new resource location resource://content-accessible/ which is referenced in .cpp files.  For example:
https://searchfox.org/mozilla-central/rev/184f0c7888dd6abb32235377693b7d1fc0b75ac1/dom/html/VideoDocument.cpp#77
So the condition can't satisfy and the browser_all_files_referenced.js test fails.

In order to make the test pass, we had a workaround to allow resource://content-accessible/ URIs.
https://searchfox.org/mozilla-central/rev/184f0c7888dd6abb32235377693b7d1fc0b75ac1/browser/base/content/test/static/browser_all_files_referenced.js#475
But the reviewer suggested that it should accept URIs for all resource packages registered in manifests, instead of matching specific patterns.

Comment 3

[Florian Quèze [:florian]]

Attachment: Fix

The fix is easy, just change the regexp to accept ressource packages that don't start with 'gre'.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8939618 [details] [diff] [review]
Fix

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

::: browser/base/content/test/static/browser_all_files_referenced.js
@@ +527,5 @@
>      }
>  
>      // Only keep strings that look like real chrome or resource urls.
>      if (/chrome:\/\/[a-zA-Z09 -]+\/(content|skin|locale)\//.test(string) ||
> +        /resource:\/\/[a-zA-Z09 -]+\/.*\.[a-z]+/.test(string))

This wants to be * instead of + on the grouping, I'm pretty sure, because `resource:///foo` is valid.

Also, you're including space (also in the chrome regex) - is that just because UTF-16 or something? If so, please indicate this in a comment. Otherwise, please remove the space from both regexes.

Comment 5

[Florian Quèze [:florian]]

Attachment: Fix v2

(In reply to :Gijs from comment #4)

Both good points, thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1956af1eacea
Test browser_all_files_referenced.js should extract URIs for all registered resource packages from libxul binary, r=Gijs.

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1956af1eacea