Closed Bug 1384389 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: General, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: cfu, Assigned: florian)

Details

Attachments

(1 file, 1 obsolete file)

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.
(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?
Flags: needinfo?(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.
Flags: needinfo?(cfu)
Attached patch Fix (obsolete) — Splinter Review
The fix is easy, just change the regexp to accept ressource packages that don't start with 'gre'.
Attachment #8939618 - Flags: review?(gijskruitbosch+bugs)
Assignee: nobody → florian
Status: NEW → ASSIGNED
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.
Attachment #8939618 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch Fix v2Splinter Review
(In reply to :Gijs from comment #4)

Both good points, thanks!
Attachment #8939618 - Attachment is obsolete: true
Pushed by florian@queze.net:
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.
https://hg.mozilla.org/mozilla-central/rev/1956af1eacea
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.