If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

netErrorURL should be resolved from start-manifest url

RESOLVED FIXED

Status

Firefox OS
Runtime
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8601329 [details] [diff] [review]
version 1
Attachment #8601329 - Flags: review?(fabrice)
(Assignee)

Comment 2

2 years ago
Created attachment 8601330 [details] [diff] [review]
version 2
Attachment #8601330 - Flags: review?(fabrice)
(Assignee)

Comment 3

2 years ago
Fabrice, we want to be able to define the network error URL as relative to the start-manifest URL.

The start-manifest URL is only accessible in the parent process.

Here are 2 versions of the same idea. One where the netErrorURL is resolved in the parent process, one where only the startManifestURL is communicated to the child process, where the full URL is resolved.

The 2nd approach might be a tiny bit better because later we might want to do something similar again (querying the startManifestURL from the parent process).

Maybe we could also store the manifest URL in a pref or a setting.
(Assignee)

Comment 4

2 years ago
Apparently, <script src="foo.js"> is not found. I'm suspecting gecko to resolve foo.js from the original URI, not the new one. I don't know how to work around that.
Attachment #8601329 - Flags: review?(fabrice)
Attachment #8601330 - Flags: review?(fabrice)
The start manifest url is in a pref, it's b2g.system_manifest_url. You should be able to just use that instead of remoting to the parent.

I faced the same url resolution issue when I did a quick hack for that. It works with app:// urls but not http(s) ones probably because of the way we treat the originalUri in the app:// protocol handler.
(Assignee)

Comment 6

2 years ago
(In reply to Fabrice Desré [:fabrice] from comment #5)
> The start manifest url is in a pref, it's b2g.system_manifest_url. You
> should be able to just use that instead of remoting to the parent.

It is not when you use --start-manifest.
(In reply to Paul Rouget [:paul] from comment #6)
> (In reply to Fabrice Desré [:fabrice] from comment #5)
> > The start manifest url is in a pref, it's b2g.system_manifest_url. You
> > should be able to just use that instead of remoting to the parent.
> 
> It is not when you use --start-manifest.

Then we should fix that.
(Assignee)

Comment 8

2 years ago
(In reply to Fabrice Desré [:fabrice] from comment #7)
> (In reply to Paul Rouget [:paul] from comment #6)
> > (In reply to Fabrice Desré [:fabrice] from comment #5)
> > > The start manifest url is in a pref, it's b2g.system_manifest_url. You
> > > should be able to just use that instead of remoting to the parent.
> > 
> > It is not when you use --start-manifest.
> 
> Then we should fix that.

That's what I'm saying in comment 3. But we can't just override b2g.system_manifest_url, so should we use a new pref name? Like b2g.effective_system_manifest_url or something…
(Assignee)

Updated

2 years ago
Flags: needinfo?(fabrice)
(Assignee)

Comment 9

2 years ago
Wait, I didn't realize there was 2 prefs. system_manifest_url and start_manifest_url.
Flags: needinfo?(fabrice)
(Assignee)

Comment 10

2 years ago
Created attachment 8602578 [details] [diff] [review]
v2

That's much better :)
Assignee: nobody → paul
Attachment #8601329 - Attachment is obsolete: true
Attachment #8601330 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8602578 - Flags: review?(fabrice)
Comment on attachment 8602578 [details] [diff] [review]
v2

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

::: b2g/components/B2GAboutRedirector.js
@@ +20,5 @@
> +  // new URL("error.html", "http://foo.com/index.html") -> http://foo.com/error.html
> +  // new URL("http://bar.com/error.html", "http://foo.com/index.html") -> http://bar.com/error.html
> +  // new URL("http://bar.com/error.html", undefined) -> http://bar.com/error.html
> +  // new URL("error.html", undefined) -> throw "error.html is not a valid URL."
> +  return new URL(netErrorURL, systemManifestURL).href;

nit: since you import Services.jsm, you could use Services.io.newURI(netErrorURL, null, systemManifestURL).spec and not have to importGlobalProperties();
Attachment #8602578 - Flags: review?(fabrice) → review+
(Assignee)

Comment 12

2 years ago
Created attachment 8603206 [details] [diff] [review]
v3

Addressed Fabrice's comment.
Attachment #8602578 - Attachment is obsolete: true
Attachment #8603206 - Flags: review+
(Assignee)

Comment 13

2 years ago
Created attachment 8604448 [details] [diff] [review]
v3 without graphene changes. r=fabrice

I'd like to land this in m-c, and land the graphene part in larch from bug 1134106.
Attachment #8603206 - Attachment is obsolete: true
Attachment #8604448 - Flags: review+
(Assignee)

Comment 14

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b59341f21cc
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Summary: Graphene: netErrorURL should be resolved from start-manifest url → netErrorURL should be resolved from start-manifest url
(Assignee)

Updated

2 years ago
Blocks: 1163905

Comment 15

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/60854c327e1c
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/60854c327e1c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.