Intent not passed to Android Component
Categories
(GeckoView :: General, defect, P1)
Tracking
(firefox78 fixed)
Tracking | Status | |
---|---|---|
firefox78 | --- | fixed |
People
(Reporter: royang51, Assigned: droeh)
References
Details
(Whiteboard: [geckoview:m78])
Attachments
(1 file)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:71.0) Gecko/20100101 Firefox/71.0
Steps to reproduce:
Click on the link "Open it" link.
Actual results:
No onLoadRequest() call to Android Component.
Expected results:
onLoadRequest() called so we can launch the deep link intent in Microsoft Team App.
Comment 1•1 year ago
|
||
don't report load requests for top level. Needs investigation. Cross Origin request blocked.
Updated•1 year ago
|
Assignee | ||
Comment 3•1 year ago
|
||
Reopening because this is not fixed by the patch for bug 1600704 -- I can reproduce this, and it's busted differently than the cases from 1600704 and similar.
Updated•1 year ago
|
Updated•11 months ago
|
Is there anything done about this? It is quite annoying as a user, since all links sent to Teams meetings have to be copied and opened in another browser.
Assignee | ||
Comment 5•11 months ago
|
||
(In reply to Mattias from comment #4)
Is there anything done about this? It is quite annoying as a user, since all links sent to Teams meetings have to be copied and opened in another browser.
Yes -- we had some major changes to make in how onLoadRequest
works that initially seemed like they would fix this (bug 1619798), but didn't. The issue is that this tries to load the msteams:
URI in an iframe, and we currently don't pass any non-top-level URI to onLoadRequest
. We're exploring some possible API changes to let us handle app links loaded in iframes while still limiting onLoadRequest
to top-level loads, so expect movement soon.
Updated•11 months ago
|
Assignee | ||
Comment 6•11 months ago
|
||
Matt, for one approach I'm considering for this it would be useful to load an error page if a promise resolves to a certain value (essentially, to have the content process hit nsDocShell:DisplayLoadError
). I was hoping supplying the right error code (NS_ERROR_UNKNOWN_PROTOCOL
) to DisconnectChildListeners
or cancelling mChannel
might work, but neither seem to. Can you think of any approach I could take here? (Is it even remotely reasonable?)
Comment 7•11 months ago
|
||
NS_ERROR_UNKNOWN_PROTOCOL sort of intentionally avoids display an error page most of the time, see https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/docshell/base/nsDocShell.cpp#6241
Using DisconnectChildListener should hit that code there, which has the set of error code that result in an error page (different for top-level vs iframes!).
We can probably change some of that behaviour too if needed.
Assignee | ||
Comment 8•11 months ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
NS_ERROR_UNKNOWN_PROTOCOL sort of intentionally avoids display an error page most of the time, see https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/docshell/base/nsDocShell.cpp#6241
Using DisconnectChildListener should hit that code there, which has the set of error code that result in an error page (different for top-level vs iframes!).
We can probably change some of that behaviour too if needed.
Quick followup: Should I be passing NS_ERROR_UNKNOWN_PROTOCOL
to both params of DisconnectChildListeners
? I'm doing that currently and not hitting nsDocShell::DisplayLoadError
at all.
Assignee | ||
Comment 9•11 months ago
|
||
(In reply to Dylan Roeh (:droeh) (he/him) from comment #8)
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
NS_ERROR_UNKNOWN_PROTOCOL sort of intentionally avoids display an error page most of the time, see https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/docshell/base/nsDocShell.cpp#6241
Using DisconnectChildListener should hit that code there, which has the set of error code that result in an error page (different for top-level vs iframes!).
We can probably change some of that behaviour too if needed.
Quick followup: Should I be passing
NS_ERROR_UNKNOWN_PROTOCOL
to both params ofDisconnectChildListeners
? I'm doing that currently and not hittingnsDocShell::DisplayLoadError
at all.
Never mind, it was a problem with my code. Looks like we're hitting it correctly now.
Updated•10 months ago
|
Assignee | ||
Comment 10•10 months ago
|
||
Comment 11•10 months ago
•
|
||
If you allow a little API feedback:
Load listeners in XUL work similarly in that they trigger for iframes. However, in the past, that has many times led to bugs in our code. We tested with a normal document, and it worked fine, and then much much later there was an iframe and it triggered the listener, and our code didn't expect that, and it broke in subtle but bad ways. This was despite the fact that this behavior was documented. We just didn't expect it and didn't think of it. It's just too surprising.
Now that we know of that "foot angle", of course we check for that. However, in almost all of our code, we always care about the top level load listeners only. So, we always have to add that check for the top level. That's a little annoying, because it clutters the code.
To avoid that, both to surprise new developers - and testers, because nobody thinks of testing this specific case -, and to avoid code clutter in the majority case, and avoid the "foot angle", here's a proposal:
Instead of modifying the existing listener, could you just add a new listener with a new name, specifically for this case? And the existing one keeps its current behavior?
Because it my experience, you almost always care only about the top level. Be it URL bar or security indicators, or progress listeners, or a load finished listener. Very rarely do we care about iframes.
Discussion on mobile-firefox-dev mailling list (direct link to the post)
Updated•10 months ago
|
Assignee | ||
Comment 12•10 months ago
|
||
(In reply to Ben Bucksch (:BenB) from comment #11)
If you allow a little API feedback:
Load listeners in XUL work similarly in that they trigger for iframes. However, in the past, that has many times led to bugs in our code. We tested with a normal document, and it worked fine, and then much much later there was an iframe and it triggered the listener, and our code didn't expect that, and it broke in subtle but bad ways. This was despite the fact that this behavior was documented. We just didn't expect it and didn't think of it. It's just too surprising.
Now that we know of that "foot angle", of course we check for that. However, in almost all of our code, we always care about the top level load listeners only. So, we always have to add that check for the top level. That's a little annoying, because it clutters the code.
To avoid that, both to surprise new developers - and testers, because nobody thinks of testing this specific case -, and to avoid code clutter in the majority case, and avoid the "foot angle", here's a proposal:
Instead of modifying the existing listener, could you just add a new listener with a new name, specifically for this case? And the existing one keeps its current behavior?
Because it my experience, you almost always care only about the top level. Be it URL bar or security indicators, or progress listeners, or a load finished listener. Very rarely do we care about iframes.
Discussion on mobile-firefox-dev mailling list (direct link to the post)
Thanks! This was really good feedback, and after discussing internally we decided to go with preserving onLoadRequest
as only being for top-level requests and adding onSubframeLoadRequest
to satisfy the requirements we have here. I'll be sending out a more detailed email to mobile-firefox-dev before the patch lands.
Comment 13•10 months ago
|
||
Hey Dylan, thanks so much, for reaching out to the community with your posting, and also for listening to the feedback! I'm glad you liked my suggestion. I'm grateful. I wish you well!
Comment 14•10 months ago
|
||
Pushed by droeh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/328927902c06 Add NavigationDelegate.onSubframeLoadRequest to handle non-top-level load requests. r=snorp,mattwoodrow,geckoview-reviewers,agi
Comment 15•10 months ago
|
||
bugherder |
Description
•