Closed Bug 858828 Opened 12 years ago Closed 12 years ago

Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host] in browser.js::getUserAgentForUriAndTab

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox22 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 23
Tracking Flags:
Tracking Status
firefox22 --- verified

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

Attachments

(2 files)

Avoid missing URI host member when special-casing user agent string for Youtube domains.
2.17 KB, patch
mfinkle
: review+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review
Always return desktop user agent if requested, even for Youtube domains.
1.62 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
Relevant code is: if (this.YOUTUBE_DOMAIN.test(aUri.host)) { // Send the phone UA to youtube if this is a tablet if (defaultUA.indexOf("Android; Mobile;") === -1) return defaultUA.replace("Android;", "Android; Mobile;"); } For URIs without a host (javascript in my case), this fails. See bz's post at https://groups.google.com/forum/#!msg/mozilla.dev.platform/FYmXl6a5OOM/RoEvfIYJjEgJ for explanation. We should be careful when querying the URI's host, but I'm not sure if we should whitelist schemes to send to Youtube, blacklist schemes to not send to Youtube, interrogate the URI via XPCOM in some way, or catch an exception. How I hit this: as part of implementing FHR on Android, we load a chrome about:healthreport that embeds an iframe containing the FHR content. That content has in-body script elements that trigger this. So: <iframe> ... <body> <script type="text/javascript" src="js/script.js"></script> </body> </iframe>
Blocks: 857419
Without attacking nsIURI with a chainsaw, I would imagine the best solution is function hostMatchesDomain(uri, domain) { let scheme = uri.scheme; if (scheme == "http" || scheme == "https") { return domain.test(uri.host); } return false; } if (hostMatchesDomain(aUri, this.YOUTUBE_DOMAIN)) { ... } This is whitelisting, because we're really only talking about HTTP here. But I think that regex is a little shady: YOUTUBE_DOMAIN: /\.?youtube\.com$/, surely that should be /^([^.]+\.)*youtube\.com$/, no?
if shady == the "test your regex tool" passed on the few tests strings I gave it, then sure! Limiting to http/https sounds like a fine change. What's the regex change handle?
(In reply to Mark Finkle (:mfinkle) from comment #2) > if shady == the "test your regex tool" passed on the few tests strings I > gave it, then sure! :D Shady because: [22:33:23.681] let re = /\.?youtube\.com$/; [22:33:23.682] undefined -- [22:33:30.016] re.test("johnyoutube.com") [22:33:30.018] true > What's the regex change handle? The regex change is to avoid matching anything other than youtube.com or its subdomains. I haven't checked the syntax, but the idea is sound.
Hmm, I also see a lot of youtu.be links these days. worth adding while we're in the neighbourhood? And a tweak: just use schemeIs: let scheme = uri.scheme; if (scheme == "http" || scheme == "https") { if (uri.schemeIs("http") || uri.schemeIs("https"))
(In reply to Mike Connor [:mconnor] from comment #4) > Hmm, I also see a lot of youtu.be links these days. worth adding while > we're in the neighbourhood? It's just a redirector; not sure if that's worth doing. > And a tweak: just use schemeIs: Y'learn something new every day.
I see no existing tests for browser.js, and I really don't want to stand up a framework for the following tests, so here's a scratchpad: /* * This is a JavaScript Scratchpad. * * Enter some JavaScript, then Right Click or choose from the Execute Menu: * 1. Run to evaluate the selected text (Cmd-R), * 2. Inspect to bring up an Object Inspector on the result (Cmd-I), or, * 3. Display to insert the result in a comment after the selection. (Cmd-L) */ r = /(^|\.)youtube\.com$/; r.test("youtube.com"); /* true */ r.test("sub.youtube.com"); /* true */ r.test("badyoutube.com"); /* false */
This also avoids returning a non-default user agent for badyoutube.com.
Attachment #735870 - Flags: review?(mark.finkle)
Comment on attachment 735871 [details] [diff] [review] Always return desktop user agent if requested, even for Youtube domains. Any reason not to always return the desktop UA when requested?
Attachment #735871 - Flags: review?(mark.finkle)
AaronMT: I think this will need QA -- can you set the appropriate flags? Thanks!
Attachment #735870 - Flags: review?(mark.finkle) → review+
Comment on attachment 735871 [details] [diff] [review] Always return desktop user agent if requested, even for Youtube domains. This optimization makes sense
Attachment #735871 - Flags: review?(mark.finkle) → review+
(In reply to Nick Alexander :nalexander from comment #10) > AaronMT: I think this will need QA -- can you set the appropriate flags? > Thanks! Have a simple test-case I could use that triggers this exception?
https://hg.mozilla.org/services/services-central/rev/6de778bb9c51 https://hg.mozilla.org/services/services-central/rev/aac45a450590
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Whiteboard: [fixed in services]
Target Milestone: --- → Firefox 23
(In reply to Aaron Train [:aaronmt] from comment #12) > (In reply to Nick Alexander :nalexander from comment #10) > > AaronMT: I think this will need QA -- can you set the appropriate flags? > > Thanks! > > Have a simple test-case I could use that triggers this exception? I really don't: I hit this while implementing about:healthreport for Android. about:healthreport is chrome content with a web-content iframe that loads Javascript, and javascript:// schemes don't have host. No idea how to hit this without such a setup. (And no idea where to put automated tests for this -- it seems we don't have any framework in place for testing browser.js.) I think if you can verify that Youtube domains do "the same thing as they used to" on tablets, that should be sufficient.
https://hg.mozilla.org/mozilla-central/rev/6de778bb9c51 https://hg.mozilla.org/mozilla-central/rev/aac45a450590
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Comment on attachment 735870 [details] [diff] [review] Avoid missing URI host member when special-casing user agent string for Youtube domains. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: maybe nothing, but it shot in the dark for bug 872423 Testing completed (on m-c, etc.): been on aurora and nightly for a while Risk to taking this patch (and alternatives if risky): low String or IDL/UUID changes made by this patch: none
Attachment #735870 - Flags: approval-mozilla-beta?
Blocks: 872423
Comment on attachment 735870 [details] [diff] [review] Avoid missing URI host member when special-casing user agent string for Youtube domains. Speculative fix for a FF22 regression, approving.
Attachment #735870 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: RESOLVED → VERIFIED
status-firefox22: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: