Open Bug 1441059 Opened 2 years ago Updated 6 days ago

onLoadRequest can change the order of loads

Categories

(GeckoView :: General, enhancement, P3)

All
Android
enhancement

Tracking

(geckoview62- wontfix, firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 affected)

REOPENED
mozilla63
Tracking Status
geckoview62 - wontfix
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- affected

People

(Reporter: jchen, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 11 obsolete files)

1.52 KB, patch
Details | Diff | Splinter Review
21.98 KB, patch
Details | Diff | Splinter Review
34.04 KB, patch
jchen
: feedback+
Details | Diff | Splinter Review
3.50 KB, patch
Details | Diff | Splinter Review
When two loads are executed in quick succession, e.g. loadUri("foo") then loadUri("bar"), we can have two pending onLoadUri calls on the Gecko stack, both waiting for a response from the UI thread:

> (stack top)
> ...
> spinEventLoopUntil(onLoadUri("bar") responds)
> handleLoadUri("bar")
> ...
> spinEventLoopUntil(onLoadUri("foo") responds)
> handleLoadUri("foo")
> ...

In this case, it's clear that handleLoadUri("foo") would not return until after handleLoadUri("bar") returns, so "bar" ends up being processed before "foo". The result is an undesirable change in the order of loads. Normally, loadUri("bar") would force loadUri("foo") to cancel, but in this case it's the opposite -- loadUri("foo") forces loadUri("bar") to cancel.
This could be P1 IMO. It breaks page load behavior in non-deterministic ways.
Summary: onLoadUri can change the order of loads → onLoadRequest can change the order of loads
Assignee: nobody → droeh
Whiteboard: [geckoview:fenix]
Is it sufficient here to take the simple route of just "canceling" (pretending the app handled) any load that isn't the most recent in LoadURIDelegate as this patch does? Or do we want to go the whole distance of preserving the results of OnLoadRequest in the appropriate order?
Attachment #8975534 - Flags: review?(nchen)
Comment on attachment 8975534 [details] [diff] [review]
Effectively cancel all but the latest LoadURIDelegate.load() call

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

If the consumer returns false for the first load, but true for the second load, the expected behavior is the first load proceeds as normal. But I think with this approach, the first load will _not_ proceed at all.

We should do this properly and make the nsILoadURIDelegate interface asynchronous.
Attachment #8975534 - Flags: review?(nchen) → review-
(In reply to Jim Chen [:jchen] [:darchons] from comment #3)
> Comment on attachment 8975534 [details] [diff] [review]
> Effectively cancel all but the latest LoadURIDelegate.load() call
> 
> Review of attachment 8975534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If the consumer returns false for the first load, but true for the second
> load, the expected behavior is the first load proceeds as normal. But I
> think with this approach, the first load will _not_ proceed at all.

Yeah, that's a good point.

> We should do this properly and make the nsILoadURIDelegate interface
> asynchronous.

Can you elaborate a bit on what you have in mind here? We rely pretty strongly on this being synchronous both for our implementation of nsIBrowserDOMWindow functions in GeckoViewNavigation.jsm and in nsDocShell::InternalLoad; it seems like either we will immediately have to block and wait for the result (in which case we don't really gain much compared to keeping LoadURIDelegate.load() synchronous), or we will have to do some fairly substantial rewriting upstream to accomodate nsILoadURIDelegate being async.
Flags: needinfo?(nchen)
Yeah nsIBrowserDOMWindow probably has to stay synchronous for now, but it's possible to make nsDocShell::InternalLoad use nsILoadURIDelegate asynchronously. You would add a callback interface as a parameter of nsILoadURIDelegate::Load. InternalLoad would call Load with a callback object before returning. The callback object would save all of the arguments to InternalLoad. When the callback is invoked, it would call InternalLoad again with those saved arguments. nsDocShell already has an InternalLoadEvent class that calls InternalLoad asynchronously, so you can probably use that for this purpose.
Flags: needinfo?(nchen)
After bug 1458258 and bug 1453501 this is the last issue we see frequently when restoring sessions and loading an URL from an Intent at the same time [in Fenix and Android Components].
Attached patch async-loadUriDelegate.patch (obsolete) — Splinter Review
This doesn't fix the issue at hand yet, just converts nsILoadURIDelegate to be async; for some reason I'm not getting the callback to the aEventDispatcher.sendRequestForResult() call in LoadURIDelegate.load(), which is what I'm currently trying to figure out.
Attachment #8975534 - Attachment is obsolete: true
Attachment #8986487 - Flags: feedback?(nchen)
Comment on attachment 8986487 [details] [diff] [review]
async-loadUriDelegate.patch

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

I don't think you need `LoadURIDelegateCallback` if you make `InternalLoadEvent` implement `nsILoadURIDelegateCallback`, and you don't need `nsDocShell::InternalPreLoad` if you add a flag that tells `nsDocShell::InternalLoad` to not use `nsILoadURIDelegate`.
Attachment #8986487 - Flags: feedback?(nchen) → feedback+
Attached patch Make nsILoadURIDelegate async (obsolete) — Splinter Review
snorp, do you mind reviewing this since Jim's on PTO? I've made all of the changes he suggested in feedback and tested to confirm the patch fixes the issue, along with cleaning up the code quite a bit compared to the last patch.
Attachment #8986487 - Attachment is obsolete: true
Attachment #8987613 - Flags: review?(snorp)
Comment on attachment 8987613 [details] [diff] [review]
Make nsILoadURIDelegate async

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

Please look into using a Promise instead of a callback interface. Also, we probably need a docshell person (bz, smaug) to look at this.

::: docshell/base/nsDocShell.cpp
@@ +1019,5 @@
>                        aFirstParty,
>                        srcdoc,
>                        sourceDocShell,
>                        baseURI,
> +                      true,     // Check for handlers

I would maybe use "Check load delegates" or similar

@@ +9162,5 @@
>      , mLoadType(aLoadType)
>      , mFirstParty(aFirstParty)
>      , mSourceDocShell(aSourceDocShell)
>      , mBaseURI(aBaseURI)
> +    , mCheckDelegates(aCheckDelegates)

mCheckLoadDelegates, perhaps?

@@ +9197,5 @@
> +  Notify(bool aHandled) override
> +  {
> +    if (aHandled) {
> +      return NS_OK;
> +    } else {

No need for 'else' clause here

::: docshell/base/nsIDocShell.idl
@@ +199,5 @@
>     * @param aBaseURI             - The base URI to be used for the load.  Set in
>     *                               srcdoc loads as it cannot otherwise be inferred
>     *                               in certain situations such as view-source.
> +   * @param aCheckDelegates      - Indicates whether we should look for an external
> +   *                               handler for the URI.

"Indicates whether we should run the nsILoadURIDelegate that was set via 'loadURIDelegate'"

::: xpcom/base/nsILoadURIDelegate.idl
@@ +41,1 @@
>    loadURI(in nsIURI aURI, in short aWhere, in long aFlags,

I think ideally this would return a Promise (jsval).
Attachment #8987613 - Flags: review?(snorp) → review+
Attached patch Make nsILoadURIDelegate async (obsolete) — Splinter Review
This incorporates all of snorp's recommended changes.
Attachment #8987613 - Attachment is obsolete: true
Attachment #8988752 - Flags: review?(snorp)
Attachment #8988752 - Flags: review?(bzbarsky)
Comment on attachment 8988752 [details] [diff] [review]
Make nsILoadURIDelegate async

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

This is looking pretty good but I think we should avoid adding arguments to InternalLoad() if we can.

::: docshell/base/nsDocShell.cpp
@@ +9222,5 @@
> +
> +  LoadURIDelegateHandler(already_AddRefed<InternalLoadEvent>&& aInternalLoadEvent)
> +    : mInternalLoadEvent(aInternalLoadEvent) {}
> +
> +  void 

whitespace

::: docshell/base/nsIDocShell.idl
@@ +221,5 @@
>                                in boolean aFirstParty,
>                                in AString aSrcdoc,
>                                in nsIDocShell aSourceDocShell,
>                                in nsIURI aBaseURI,
> +                              in boolean aCheckLoadDelegates,

I still like a flag for this instead. This method already has a billion arguments as it is.

::: mobile/android/modules/geckoview/LoadURIDelegate.jsm
@@ +16,5 @@
>    // Delegate URI loading to the app.
>    // Return whether the loading has been handled.
>    load: function(aWindow, aEventDispatcher, aUri, aWhere, aFlags) {
> +    return new Promise((resolve, reject) => {
> +      if (!aWindow) {

You may find it a little nicer to do this check outside of the Promise callback. That way you can just `return Promise.resolve(false)`

@@ +28,5 @@
> +        where: aWhere,
> +        flags: aFlags
> +      };
> +
> +      aEventDispatcher.sendRequestForResult(message).then(response => {

Ah, you actually don't need to construct a new Promise at all. Just chain off of this one.

return aEventDispatcher.sendRequestForResult(message).catch(() => {
    return false;
});
Attachment #8988752 - Flags: review?(snorp) → review-
I assume this is somewhat urgent and shouldn't wait on Kyle Machulis's refactoring of InternalLoad?  In either case, please coordinate with him a bit...
I'm ok with a load flag here, as recommended in Comment 12. That'll still fit fine with the internalLoad changes I'm making in Bug 1471711.
(In reply to Kyle Machulis [:qdot] [:kmachulis] (if a patch has no decent commit message, automatic r-) from comment #14)
> I'm ok with a load flag here, as recommended in Comment 12. That'll still
> fit fine with the internalLoad changes I'm making in Bug 1471711.

Alright, will do. Let me know if you need any other changes.
Attached patch Make nsILoadURIDelegate async (obsolete) — Splinter Review
Updated to use a load flag instead of an extra param for InternalLoad; also fixes snorp's nits.
Attachment #8988752 - Attachment is obsolete: true
Attachment #8988752 - Flags: review?(bzbarsky)
Attachment #8988877 - Flags: review?(snorp)
Attachment #8988877 - Flags: review?(bzbarsky)
Comment on attachment 8988877 [details] [diff] [review]
Make nsILoadURIDelegate async

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

::: docshell/base/nsDocShell.cpp
@@ +9485,5 @@
>  
>    const bool isDocumentAuxSandboxed = doc &&
>      (doc->GetSandboxFlags() & SANDBOXED_AUXILIARY_NAVIGATION);
>  
> +  const bool checkLoadDelegates = !(aFlags & INTERNAL_LOAD_FLAGS_DELEGATES_CHECKED);

I was thinking you would have something like INTERNAL_LOAD_FLAGS_NO_DELEGATES which you would add in InternalLoadEvent::Run(), but I guess this works too.

::: mobile/android/modules/geckoview/LoadURIDelegate.jsm
@@ +27,5 @@
>        flags: aFlags
>      };
>  
> +    return aEventDispatcher.sendRequestForResult(message).catch(() => {
> +      return Promise.resolve(false);

I think you can just do `return false` here.
Attachment #8988877 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #17)
> Comment on attachment 8988877 [details] [diff] [review]
> Make nsILoadURIDelegate async
> 
> Review of attachment 8988877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +9485,5 @@
> >  
> >    const bool isDocumentAuxSandboxed = doc &&
> >      (doc->GetSandboxFlags() & SANDBOXED_AUXILIARY_NAVIGATION);
> >  
> > +  const bool checkLoadDelegates = !(aFlags & INTERNAL_LOAD_FLAGS_DELEGATES_CHECKED);
> 
> I was thinking you would have something like
> INTERNAL_LOAD_FLAGS_NO_DELEGATES which you would add in
> InternalLoadEvent::Run(), but I guess this works too.

I think that would be safe right now, but I don't want to assume any future usage of InternalLoadEvent has gone through checking for load delegates.

> ::: mobile/android/modules/geckoview/LoadURIDelegate.jsm
> @@ +27,5 @@
> >        flags: aFlags
> >      };
> >  
> > +    return aEventDispatcher.sendRequestForResult(message).catch(() => {
> > +      return Promise.resolve(false);
> 
> I think you can just do `return false` here.

Yup, you're right.
Make sure this doesn't break popup blocking, as explained at [1]. If it does, I think you'd need something similar to what wedo in `OnLinkClickEvent::Run`

[1] https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#13201
(In reply to (pto) Jim Chen [:jchen] [:darchons] from comment #19)
> Make sure this doesn't break popup blocking, as explained at [1]. If it
> does, I think you'd need something similar to what wedo in
> `OnLinkClickEvent::Run`
> 
> [1]
> https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#13201

This doesn't appear to be an issue as far as I can tell from manual testing.
I'm sorry for the horrible lag here... Still digging out from vacation mail.  :(
Comment on attachment 8988877 [details] [diff] [review]
Make nsILoadURIDelegate async

>+  NS_DECL_ISUPPORTS

This class is holding a strong ref to the InternalLoadEvent.  The event holds strong references to various stuff.  Nothing says the Promise will ever settle (or that AppendNativeHandler will append anything, for that matter).

Or to put another way, this class needs to be cycle-collected.  And InternalLoadEvent needs to become cycle-collected as well.

>     rv = mLoadURIDelegate->LoadURI(aURI, where, aFlags, aTriggeringPrincipal,

After this call, shouldn't "rv" be checked before trying to work with "promise", not after?  If rv is failure, then the promise->AppendNativeHandler(handler) call is a crash at best.

Realistically, depending on what the JS does you could have a success rv here and still a null promise....

>+  const long INTERNAL_LOAD_FLAGS_DELEGATES_CHECKED       = 0x400;

I wonder why bug 1439013 added 0x1000 but skipped 0x400.  Maybe check with the patch author there?

>+    return aEventDispatcher.sendRequestForResult(message).catch(() => {
>+      return Promise.resolve(false);
>     });

Is there a reason this isn't the simpler

  return aEventDispatcher.sendRequestForResult(message).catch(() => false);

?

>+++ b/xpcom/base/nsILoadURIDelegate.idl

This needs to document what values the returned Promise may be resolved with and what they mean, as well as what a rejection means.
Attachment #8988877 - Flags: review?(bzbarsky) → review-
Attached patch Make nsILoadURIDelegate async (obsolete) — Splinter Review
Thanks for the feedback. I'm not super familiar with the cycle collecting macros, but hopefully came up with something reasonable here. I think I've addressed all issues except the value of the flag in nsDocShell.idl -- unfortunately, Eugen (the author of the patch for bug 1439103) is on PTO for an extended period. I can't see any reason why 0x400 (or 0x800 for that matter) would be verboten from looking at his patch, but I've made a note to bring it up with him when he's back. I left 0x400 in as the value, but I can switch to 0x2000 if you'd rather play it safe.
Attachment #8988877 - Attachment is obsolete: true
Attachment #8991051 - Flags: review?(bzbarsky)
Comment on attachment 8991051 [details] [diff] [review]
Make nsILoadURIDelegate async

Hmm.  So for InternalLoadEvent that gives it two refcount members: one from Runnable (threadsafe) and one that NS_DECL_CYCLE_COLLECTING_ISUPPORTS adds (non-threadsafe, CC-enabled).  I'm not 100% convinced whether that all will work right, and it's certainly confusing.

How much pain would it be to move all the actual logic in InternalLoadEvent into a separate struct that InternalLoadEvent has as a member (directly, not as a pointer) and that your new PromiseNativeHandler will also have as a member?  Then the refcounting can differ for the two classes but they can both delegate the work to the shared struct.
Flags: needinfo?(droeh)
That seems like it should be pretty reasonable, I'll ping you if I run into any unforeseen problems with it.
Flags: needinfo?(droeh)
[geckoview:klar:p1] because this bug is a Focus+GV blocker.
Whiteboard: [geckoview:fenix] → [geckoview:fenix][geckoview:klar:p1]
Attached patch Make nsILoadURIDelegate async (obsolete) — Splinter Review
Updated per your suggestion.
Attachment #8991051 - Attachment is obsolete: true
Attachment #8991051 - Flags: review?(bzbarsky)
Attachment #8991937 - Flags: review?(bzbarsky)
Comment on attachment 8991937 [details] [diff] [review]
Make nsILoadURIDelegate async

>+  InternalLoadEvent(InternalLoadData aLoadData)

This should take the arguments it used to take and just pass them through to the mLoadData constructor, instead of copying the big struct.

>+  LoadURIDelegateHandler(InternalLoadData aLoadData)

And this should take those same args.

>+NS_IMPL_CYCLE_COLLECTION(LoadURIDelegateHandler)

I expect this doesn't compile...

In any case, this needs to actually declare to the CC the members of mLoadData that hold strong references to stuff.

>+      if (promise) {

Might as well check that before creating the LoadURIDelegateHandler.  So:

  if (NS_SUCCEEDED(rv) && promise) {
Attachment #8991937 - Flags: review?(bzbarsky) → review-
Quick question: one of the members of InternalLoadData is a Maybe<nsCOMPtr> -- is there a good/idiomatic way of declaring this to the CC? At the moment all I can come up with is to store the underlying value of the Maybe (if it exists) along with a couple of booleans indicating whether the Maybe itself exists and if it satisfies isSome(), then declare the underlying value to the CC and reconstruct the Maybe when calling InternalLoad().
Flags: needinfo?(bzbarsky)
The most idiomatic way is to create overloads of ImplCycleCollectionTraverse and ImplCycleCollectionUnlink in Maybe.h that take a Maybe argument and do the right thing.  You can see examples of how this is done for Nullable in dom/bindings/Nullable.h

I didn't realize we didn't have those overloads already!
Flags: needinfo?(bzbarsky)
Attached patch Make nsILoadURIDelegate async (obsolete) — Splinter Review
Updated per feedback
Attachment #8991937 - Attachment is obsolete: true
Attachment #8992675 - Flags: review?(bzbarsky)
This overloads ImplCycleCollectionTraverse and ImplCycleCollectionUnlink for Maybe.
Attachment #8992677 - Flags: review?(bzbarsky)
Comment on attachment 8992677 [details] [diff] [review]
Add cycle collection implementations to Maybe

r=me; this one should land before the other changeset, of course.
Attachment #8992677 - Flags: review?(bzbarsky) → review+
Comment on attachment 8992675 [details] [diff] [review]
Make nsILoadURIDelegate async

>+  nsresult Run() {

Please leave curly on separate line.

>+                aBaseURI) {}

Curlies on next line, please.

r=me.  Sorry for the lag on this round, and thank you for bearing with this!
Attachment #8992675 - Flags: review?(bzbarsky) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bd4f224f9a
Add cycle collection implementations for Maybe. r=bz
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8f58cb7315
Make nsILoadURIDelegate async to preserve the order of GeckoSession.loadUri() calls. r=snorp,bz
https://hg.mozilla.org/mozilla-central/rev/f7bd4f224f9a
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Backed out changeset 9a8f58cb7315 (bug 1441059) for geckoview failures on multiple files.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9884ab1267e546a1a2fc6c653c4d5b095329b0

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9a8f58cb7315a92f9ebede217e1b8d968c5e5a1b&selectedJob=189563360

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189563360&repo=mozilla-inbound&lineNumber=1549

[task 2018-07-23T16:32:55.290Z] 16:32:55     INFO -  TEST-START | org.mozilla.geckoview.test.AccessibilityTest.testCheckbox
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: id=AndroidJUnitRunner
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: current=11
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: class=org.mozilla.geckoview.test.AccessibilityTest
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stream=
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | Error in testCheckbox(org.mozilla.geckoview.test.AccessibilityTest):
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutException: Timed out after 120000ms
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutRunnable.run(UiThreadUtils.java:48)
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Handler.handleCallback(Handler.java:730)
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Handler.dispatchMessage(Handler.java:92)
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.util.UiThreadUtils.loopUntilIdle(UiThreadUtils.java:100)
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitUntilCalled(GeckoSessionTestRule.java:1735)
[task 2018-07-23T16:35:21.672Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStops(GeckoSessionTestRule.java:1540)
[task 2018-07-23T16:35:21.673Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStop(GeckoSessionTestRule.java:1515)
[task 2018-07-23T16:35:21.674Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStop(GeckoSessionTestRule.java:1505)
[task 2018-07-23T16:35:21.675Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.AccessibilityTest.testCheckbox(AccessibilityTest.kt:386)
[task 2018-07-23T16:35:21.675Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invokeNative(Native Method)
[task 2018-07-23T16:35:21.676Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invoke(Method.java:525)
[task 2018-07-23T16:35:21.676Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
[task 2018-07-23T16:35:21.677Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
[task 2018-07-23T16:35:21.678Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
[task 2018-07-23T16:35:21.678Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
[task 2018-07-23T16:35:21.679Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
[task 2018-07-23T16:35:21.679Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
[task 2018-07-23T16:35:21.680Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule$4.evaluate(GeckoSessionTestRule.java:1486)
[task 2018-07-23T16:35:21.680Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.support.test.internal.statement.UiThreadStatement$1.run(UiThreadStatement.java:44)
[task 2018-07-23T16:35:21.681Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1719)
[task 2018-07-23T16:35:21.681Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Handler.handleCallback(Handler.java:730)
[task 2018-07-23T16:35:21.682Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Handler.dispatchMessage(Handler.java:92)
[task 2018-07-23T16:35:21.682Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Looper.loop(Looper.java:137)
[task 2018-07-23T16:35:21.682Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.app.ActivityThread.main(ActivityThread.java:5103)
[task 2018-07-23T16:35:21.683Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invokeNative(Native Method)
[task 2018-07-23T16:35:21.683Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invoke(Method.java:525)
[task 2018-07-23T16:35:21.684Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
[task 2018-07-23T16:35:21.685Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
[task 2018-07-23T16:35:21.685Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at dalvik.system.NativeStart.main(Native Method)
[task 2018-07-23T16:35:21.686Z] 16:35:21     INFO -  org.mozilla.geckoview.test |
[task 2018-07-23T16:35:21.686Z] 16:35:21     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: numtests=163
[task 2018-07-23T16:35:21.687Z] 16:35:21     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stack=org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutException: Timed out after 120000ms
[task 2018-07-23T16:35:21.688Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.util.UiThreadUtils$TimeoutRunnable.run(UiThreadUtils.java:48)
[task 2018-07-23T16:35:21.688Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Handler.handleCallback(Handler.java:730)
[task 2018-07-23T16:35:21.689Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Handler.dispatchMessage(Handler.java:92)
[task 2018-07-23T16:35:21.689Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.util.UiThreadUtils.loopUntilIdle(UiThreadUtils.java:100)
[task 2018-07-23T16:35:21.690Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitUntilCalled(GeckoSessionTestRule.java:1735)
[task 2018-07-23T16:35:21.691Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStops(GeckoSessionTestRule.java:1540)
[task 2018-07-23T16:35:21.691Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStop(GeckoSessionTestRule.java:1515)
[task 2018-07-23T16:35:21.692Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.waitForPageStop(GeckoSessionTestRule.java:1505)
[task 2018-07-23T16:35:21.692Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.AccessibilityTest.testCheckbox(AccessibilityTest.kt:386)
[task 2018-07-23T16:35:21.693Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invokeNative(Native Method)
[task 2018-07-23T16:35:21.694Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invoke(Method.java:525)
[task 2018-07-23T16:35:21.694Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
[task 2018-07-23T16:35:21.695Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
[task 2018-07-23T16:35:21.695Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
[task 2018-07-23T16:35:21.696Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
[task 2018-07-23T16:35:21.696Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
[task 2018-07-23T16:35:21.697Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
[task 2018-07-23T16:35:21.698Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at org.mozilla.geckoview.test.rule.GeckoSessionTestRule$4.evaluate(GeckoSessionTestRule.java:1486)
[task 2018-07-23T16:35:21.699Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.support.test.internal.statement.UiThreadStatement$1.run(UiThreadStatement.java:44)
[task 2018-07-23T16:35:21.699Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1719)
[task 2018-07-23T16:35:21.699Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Handler.handleCallback(Handler.java:730)
[task 2018-07-23T16:35:21.700Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Handler.dispatchMessage(Handler.java:92)
[task 2018-07-23T16:35:21.700Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.os.Looper.loop(Looper.java:137)
[task 2018-07-23T16:35:21.701Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at android.app.ActivityThread.main(ActivityThread.java:5103)
[task 2018-07-23T16:35:21.701Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invokeNative(Native Method)
[task 2018-07-23T16:35:21.702Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at java.lang.reflect.Method.invoke(Method.java:525)
[task 2018-07-23T16:35:21.703Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
[task 2018-07-23T16:35:21.703Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
[task 2018-07-23T16:35:21.704Z] 16:35:21     INFO -  org.mozilla.geckoview.test | 	at dalvik.system.NativeStart.main(Native Method)
[task 2018-07-23T16:35:21.704Z] 16:35:21     INFO -  org.mozilla.geckoview.test |
[task 2018-07-23T16:35:21.704Z] 16:35:21     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=testCheckbox
[task 2018-07-23T16:35:21.705Z] 16:35:21     INFO -  org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: -2
[task 2018-07-23T16:35:21.705Z] 16:35:21  WARNING -  TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.AccessibilityTest.testCheckbox | status -2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 63 → ---
We decided that this bug does not need to block Focus+GV if the redirect bug doesn't block.
Whiteboard: [geckoview:fenix][geckoview:klar:p1] → [geckoview:fenix][geckoview:klar:p2]
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e66e6bd82e3f
Make nsILoadURIDelegate async to preserve the order of GeckoSession.loadUri() calls. r=snorp,bz
https://hg.mozilla.org/mozilla-central/rev/e66e6bd82e3f
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
status-firefox62=wontfix because, IIUC, we don't need to uplift this fix to 62 Beta.
(In reply to Jim Chen [:jchen] [:darchons] from comment #39)
> Dylan can you land a follow-up to re-enable the "multipleLoads" test? [1]
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> ad36eff63e208b37bc9441b91b7cea7291d82890/mobile/android/geckoview/src/
> androidTest/java/org/mozilla/geckoview/test/ProgressDelegateTest.kt#89

I just rolled this change into the patch when I landed it.
Flags: needinfo?(droeh)
Comment on attachment 8992675 [details] [diff] [review]
Make nsILoadURIDelegate async

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for consideration: N/A

User impact if declined: We won't be able to uplift the patch for bug 1478171, certain redirects may not work in GeckoView

Fix Landed on Version: 63

Risk to taking this patch (and alternatives if risky): Should not impact Firefox/Fennec at all, solely GeckoView. Should be relatively low risk.

String or UUID changes made by this patch: N/A

See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.
Attachment #8992675 - Flags: approval-mozilla-geckoview62?
Comment on attachment 8992677 [details] [diff] [review]
Add cycle collection implementations to Maybe

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for consideration: N/A

User impact if declined: We won't be able to land the patch for bug 1478171, some redirects may not work correctly in GeckoView apps.

Fix Landed on Version: 63

Risk to taking this patch (and alternatives if risky): Very low, just adds cycle collection implementations for Maybe.

String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/Uplift_rules for more info.
Attachment #8992677 - Flags: approval-mozilla-geckoview62?
Comment on attachment 8992675 [details] [diff] [review]
Make nsILoadURIDelegate async

GV change, needed for Focus release
Attachment #8992675 - Flags: approval-mozilla-geckoview62? → approval-mozilla-geckoview62+
Comment on attachment 8992677 [details] [diff] [review]
Add cycle collection implementations to Maybe

GV change, needed for Focus release
Attachment #8992677 - Flags: approval-mozilla-geckoview62? → approval-mozilla-geckoview62+
Depends on: 1489257
Attachment #8992675 - Flags: approval-mozilla-geckoview62+ → approval-mozilla-geckoview62-
Comment on attachment 8992677 [details] [diff] [review]
Add cycle collection implementations to Maybe

Per IRC discussion with droeh.
Attachment #8992677 - Flags: approval-mozilla-geckoview62+ → approval-mozilla-geckoview62-
Backed out in bug 1489257
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Jim, do you have any suggestions on how to approach the HTMLFormElement problem we ran into?
Flags: needinfo?(nchen)
If you look at what `HTMLFormElement` does with the docshell/request, it mostly uses them to register an `nsIWebProgressListener` [1]. To fix that, instead of having `InternalLoad` return a docshell/request, I think we want to make it accept an `nsIWebProgressListener`, so the call becomes completely async. Essentially, you want `nsDocShell` to implement the block of code at [1] using an `nsIWebProgressListener` that `HTMLFormElement` passes in. You can ask :bz or :smaug too on their thoughts.
Flags: needinfo?(nchen)
Attached patch Make nsILoadURIDelegate async (obsolete) — Splinter Review
Thanks; it's a bit more involved than that because we need to be able to call ForgetCurrentSubmission on the HTMLFormElement when necessary if we can't rely on synchronously getting a docshell outparam. I have something working (tested manually with Google logins) here, but it's a bit uglier than I'd like.

(For bz: the original patch you reviewed for this had to be backed out because HTMLFormElement::SubmitSubmission() relies on synchronously getting the docshell outparam from InternalLoad, which ended up breaking all google login pages.)
Attachment #8992675 - Attachment is obsolete: true
Attachment #8992677 - Attachment is obsolete: true
Attachment #9012394 - Flags: review?(nchen)
Attachment #9012394 - Flags: review?(bzbarsky)
Comment on attachment 9012394 [details] [diff] [review]
Make nsILoadURIDelegate async

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

I was thinking you would go through `nsIWebProgressListener` for all interaction between `nsDocShell` and `HTMLFormElement`, so that `HTMLFormElement` would not see the `nsIDocShell` or `nsIRequest` at all. For example, the main reason for calling `ForgetCurrentSubmission` is because `HTMLFormElement` keeps a reference to a `nsIRequest`, but if `HTMLFormElement` doesn't see the `nsIRequest` at all, it wouldn't need `ForgetCurrentSubmission`.
Attachment #9012394 - Flags: review?(nchen) → feedback+
(In reply to Jim Chen [:jchen] [:darchons] from comment #60)
> Comment on attachment 9012394 [details] [diff] [review]
> Make nsILoadURIDelegate async
> 
> Review of attachment 9012394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I was thinking you would go through `nsIWebProgressListener` for all
> interaction between `nsDocShell` and `HTMLFormElement`, so that
> `HTMLFormElement` would not see the `nsIDocShell` or `nsIRequest` at all.
> For example, the main reason for calling `ForgetCurrentSubmission` is
> because `HTMLFormElement` keeps a reference to a `nsIRequest`, but if
> `HTMLFormElement` doesn't see the `nsIRequest` at all, it wouldn't need
> `ForgetCurrentSubmission`.

Maybe I'm just misunderstanding something, but this doesn't make sense to me at all. In my understanding:

* ForgetCurrentSubmission is important for ensuring that mIsSubmitting and mNotifiedObservers are correct and removing the progress listener; freeing the reference to the nsIRequest is just a byproduct.
* HTMLFormElement only keeps a reference to the nsIRequest to make sure that it can call ForgetCurrentSubmission in response to STATE_STOP for the *correct* request. Without this, the request could just be confined to SubmitSubmission() as far as I can tell.

I don't see how we can ensure that mIsSubmitting and mNotifiedObservers are properly updated with what you're suggesting, and both serve important purposes.
Flags: needinfo?(nchen)
What I meant was `nsDocShell` can always call `nsIWebProgressListener::OnStateChange` with STATE_STOP in situations where `HTMLFormElement` currently calls `ForgetCurrentSubmission`. So in effect `ForgetCurrentSubmission` is still called but indirectly.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #62)
> What I meant was `nsDocShell` can always call
> `nsIWebProgressListener::OnStateChange` with STATE_STOP in situations where
> `HTMLFormElement` currently calls `ForgetCurrentSubmission`. So in effect
> `ForgetCurrentSubmission` is still called but indirectly.

I still don't think that's viable, though? HTMLFormElement only wants to call ForgetCurrentSubmission in response to STATE_STOP for a particular request (the one created by InternalLoad), and it seems like there may be cases where InternalLoad returns NS_OK with aRequest set to null, but we still want to immediately call ForgetCurrentSubmission -- which doesn't seem possible to do correctly via nsIWebProgressListener.

Essentially,
* We'd still need to store the nsIRequest in HTMLFormElement to make sure we're calling ForgetCurrentSubmission in response to STATE_STOP for the correct request
* There are (I think) cases where InternalLoad succeeds but does not create a request, in which case we still need to call ForgetCurrentSubmission

Let's talk on Slack on Monday, I guess.
Comment on attachment 9012394 [details] [diff] [review]
Make nsILoadURIDelegate async

Canceling review request until I update per jim's suggestions.
Attachment #9012394 - Flags: review?(bzbarsky)
Attached patch Make nsILoadURIDelegate async (obsolete) — Splinter Review
Here's a first pass at what we talked about. Some issues:

First, it doesn't actually work -- behavior on google logins is the same as before.
Second, I'm not clear on what the advantage of using the progress listener to communicate is; to add the HTMLFormElement as a listener we need a reference to it in InternalLoad(), at which point we might as well just call ForgetSubmission() directly -- am I missing something here?
Attachment #9012394 - Attachment is obsolete: true
Attachment #9013695 - Flags: feedback?(nchen)
Comment on attachment 9013695 [details] [diff] [review]
Make nsILoadURIDelegate async

Got feedback from Jim on slack, updating accordingly.
Attachment #9013695 - Flags: feedback?(nchen)
Alright, Jim, I think you had in mind something like this? Unfortunately, it's still not actually working -- we get the same behavior on Google login pages with or without this patch (as long as patches 1&2 are applied). I'm investigating further to try and figure out what's going on, but if you've got any ideas or you see anything I've missed in this patch please let me know.
Attachment #9015351 - Flags: feedback?(nchen)
Comment on attachment 9015351 [details] [diff] [review]
3.0 - Make onLinkClickSync async

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

Have you stepped through the code to see when `ForgetCurrentSubmission` is actually getting called?

I think instead of spreading `OnStateChange` around everywhere, we should check in `OnLinkClickSync` if the load was async. If the load was not async, we call `OnStateChange` from `OnLinkClickSync` only.

::: docshell/base/nsDocShell.cpp
@@ +9811,5 @@
>  
>      // Else we ran out of memory, or were a popup and got blocked,
>      // or something.
> +    if (NS_SUCCEEDED(rv) && aWebProgressListener) {
> +      aWebProgressListener->OnStateChange(nullptr, nullptr, 

This isn't quite right, because the `InternalLoad` call above may have succeeded, in which case we shouldn't signal the state change at this point.
Attachment #9015351 - Flags: feedback?(nchen) → feedback+
63=wontfix because launching app links (like Reddit's) is a low priority for Focus.
(In reply to Jim Chen [:jchen] [:darchons] from comment #70)
> Comment on attachment 9015351 [details] [diff] [review]
> 3.0 - Make onLinkClickSync async
> 
> Review of attachment 9015351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Have you stepped through the code to see when `ForgetCurrentSubmission` is
> actually getting called?

Yeah, it looks to me like we are getting only the expected ForgetCurrentSubmission call from InternalLoad/OnLinkClickSync, but I'm seeing an extraneous ForgetCurrentSubmission call coming from HTMLFormElement::UnbindFromTree compared to when I go through a Google login without these patches applied, so there may be something subtler going on here. 

bz, can you offer any insight here?
Flags: needinfo?(bzbarsky)
Sorry for the lag here; I was trying to page this in.

As far as the ForgetCurrentSubmission call from UnbindFromTree, that's a bit odd.  I'd would expect the site is removing the form from the DOM, or not, independently from these patches.

For the overall problem, it seems to me that ideally we would align closer to the spec here.  Per spec we would queue a task to do the submission navigation, then cancel that task if a new submit attempt happens and not have to keep track of any loads.  That's different from our current setup where the later attempts are ignored instead of the earlier ones being dropped if there are multiple submit attempts.  That said, I can't tell whether the spec would effectively suffer from bug 72906.  I suspect it might, because the spec's "planned navigation" task would run before the second click happens.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Flags: needinfo?(annevk)
Hmm, submission handling... reminds me of bug 1495363, which I need to still give feedback.
bz, I'm not sure. It also depends on how much in "navigate" is done synchronously as unloading might cause the second navigation attempt to end up ignored.
Flags: needinfo?(annevk)
Priority: P1 → P2
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 63 → mozilla63
Whiteboard: [geckoview:fenix][geckoview:klar:p2] → [geckoview:fenix:p2]

smaug and droeh, what status of this bug?

Since LoadURIDelegate in GV uses event loop, some strange behavior occurs such as bug 1511154. Bug 1511154 issue is that location.hash setter is called in touch event handler of user content, then it processes other events due to event loop in GV's LoadURIDelegate, so unexpected behavior occurs.

Flags: needinfo?(droeh)

(In reply to Makoto Kato [:m_kato] from comment #76)

smaug and droeh, what status of this bug?

Since LoadURIDelegate in GV uses event loop, some strange behavior occurs such as bug 1511154. Bug 1511154 issue is that location.hash setter is called in touch event handler of user content, then it processes other events due to event loop in GV's LoadURIDelegate, so unexpected behavior occurs.

I'm pretty much stalled on it. I have some alternate ideas to address the specific issue here (where we change the order of loads), but they'd leave LoadURIDelegate spinning the event loop, so it sounds like they won't help with bug 1511154. I did suggest to snorp some time ago that we might be better off just making onLoadRequest a synchronous API -- maybe we should consider revisiting that?

Flags: needinfo?(droeh) → needinfo?(snorp)

Quick update: I talked with snorp about this on Slack and he convinced me there's a viable approach to keeping the async API and not spinning the event loop; whichever of us finds time first will work on implementing that.

Flags: needinfo?(snorp)

Adding another bug this blocks and removing myself as assignee as I have no clear way forward on this. In my opinion this needs either an entirely new (GV-specific) approach or serious involvement from the core Gecko side of things to make InternalLoad async.

Assignee: droeh → nobody
Blocks: 1557723
Priority: P2 → P3
Whiteboard: [geckoview:fenix:p2]
Duplicate of this bug: 1557723

I had an idea for a totally different approach here that initially seemed to have some promise; listening for http-on-modify-request and using nsIRequest.suspend() (and .resume()/.cancel()) to wait for GV's response. Unfortunately I don't see any obvious way to get the window target to GV in this approach. Putting up this WIP in case anyone's interested or has any ideas on that.

You need to log in before you can comment on or make changes to this bug.