1441059 - onLoadRequest can change the order of loads (don't spin the event loop)

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

This could be P1 IMO. It breaks page load behavior in non-deterministic ways.

Comment 2

[Dylan Roeh (:droeh) (he/him)]

Attachment: Effectively cancel all but the latest LoadURIDelegate.load() call

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?

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 4

[Dylan Roeh (:droeh) (he/him)]

(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.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

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].

Comment 7

[Dylan Roeh (:droeh) (he/him)]

Attachment: async-loadUriDelegate.patch

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.

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

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`.

Comment 9

[Dylan Roeh (:droeh) (he/him)]

Attachment: Make nsILoadURIDelegate async

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.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

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).

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Attachment: Make nsILoadURIDelegate async

This incorporates all of snorp's recommended changes.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

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;
});

Comment 13

[Boris Zbarsky [:bzbarsky]]

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...

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 15

[Dylan Roeh (:droeh) (he/him)]

(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.

Comment 16

[Dylan Roeh (:droeh) (he/him)]

Attachment: Make nsILoadURIDelegate async

Updated to use a load flag instead of an extra param for InternalLoad; also fixes snorp's nits.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 18

[Dylan Roeh (:droeh) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 20

[Dylan Roeh (:droeh) (he/him)]

(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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the horrible lag here... Still digging out from vacation mail.  :(

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Dylan Roeh (:droeh) (he/him)]

Attachment: Make nsILoadURIDelegate async

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.

Comment 24

[Boris Zbarsky [: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.

Comment 25

[Dylan Roeh (:droeh) (he/him)]

That seems like it should be pretty reasonable, I'll ping you if I run into any unforeseen problems with it.

Comment 26

[Chris Peterson [:cpeterson]]

[geckoview:klar:p1] because this bug is a Focus+GV blocker.

Comment 27

[Dylan Roeh (:droeh) (he/him)]

Attachment: Make nsILoadURIDelegate async

Updated per your suggestion.

Comment 28

[Boris Zbarsky [: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) {

Comment 29

[Dylan Roeh (:droeh) (he/him)]

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().

Comment 30

[Boris Zbarsky [: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!

Comment 31

[Dylan Roeh (:droeh) (he/him)]

Attachment: Make nsILoadURIDelegate async

Updated per feedback

Comment 32

[Dylan Roeh (:droeh) (he/him)]

Attachment: Add cycle collection implementations to Maybe

This overloads ImplCycleCollectionTraverse and ImplCycleCollectionUnlink for Maybe.

Comment 33

[Boris Zbarsky [: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.

Comment 34

[Boris Zbarsky [:bzbarsky]]

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!

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7bd4f224f9a
Add cycle collection implementations for Maybe. r=bz

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8f58cb7315
Make nsILoadURIDelegate async to preserve the order of GeckoSession.loadUri() calls. r=snorp,bz

Comment 37

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f7bd4f224f9a

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c635e583eeb3
Fix eslint failure. r=me on CLOSED TREE

Comment 39

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 40

[Cristian Brindusan [:cbrindusan]]

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

Comment 41

[Cristian Brindusan [:cbrindusan]]

Forgot to backout the follow-up related to ESlint failure:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c635e583eeb3a483d664a37bdadb1a9c3c41f1d4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified 

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

Comment 42

[Chris Peterson [:cpeterson]]

We decided that this bug does not need to block Focus+GV if the redirect bug doesn't block.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e66e6bd82e3f
Make nsILoadURIDelegate async to preserve the order of GeckoSession.loadUri() calls. r=snorp,bz

Comment 44

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/e66e6bd82e3f

Comment 45

[Chris Peterson [:cpeterson]]

status-firefox62=wontfix because, IIUC, we don't need to uplift this fix to 62 Beta.

Comment 46

[Dylan Roeh (:droeh) (he/him)]

(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.

Comment 47

[Dylan Roeh (:droeh) (he/him)]

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.

Comment 48

[Dylan Roeh (:droeh) (he/him)]

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.

Comment 49

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8992675 [details] [diff] [review]
Make nsILoadURIDelegate async

GV change, needed for Focus release

Comment 50

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8992677 [details] [diff] [review]
Add cycle collection implementations to Maybe

GV change, needed for Focus release

Comment 51

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/releases/mozilla-release/rev/f35ce4a936985ba0c352cc783721cbf142e730ca

Comment 52

[(inactive) Jim Chen [:jchen] [:darchons]]

Fix lint error and uplift part 1 to fix bustage

https://hg.mozilla.org/releases/mozilla-release/rev/76f94cb522c7e386c55491311f901a9dd96d6ec2
https://hg.mozilla.org/releases/mozilla-release/rev/427eacc1a0bfc1bd07f9c41e16d9d03a739dce7c

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Backed out from GECKOVIEW_62_RELBRANCH for causing bug 1489257.
https://hg.mozilla.org/releases/mozilla-release/rev/4cdeecd31350b13f8741202dc06d87d8bc3d84db

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8992677 [details] [diff] [review]
Add cycle collection implementations to Maybe

Per IRC discussion with droeh.

Comment 55

[(inactive) Jim Chen [:jchen] [:darchons]]

Backed out in bug 1489257

Comment 56

[Dylan Roeh (:droeh) (he/him)]

Jim, do you have any suggestions on how to approach the HTMLFormElement problem we ran into?

Comment 57

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 58

[(inactive) Jim Chen [:jchen] [:darchons]]

[1] https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/dom/html/HTMLFormElement.cpp#764-783

Comment 59

[Dylan Roeh (:droeh) (he/him)]

Attachment: Make nsILoadURIDelegate async

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.)

Comment 60

[(inactive) Jim Chen [:jchen] [:darchons]]

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`.

Comment 61

[Dylan Roeh (:droeh) (he/him)]

(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.

Comment 62

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 63

[Dylan Roeh (:droeh) (he/him)]

(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 64

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9012394 [details] [diff] [review]
Make nsILoadURIDelegate async

Canceling review request until I update per jim's suggestions.

Comment 65

[Dylan Roeh (:droeh) (he/him)]

Attachment: Make nsILoadURIDelegate async

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?

Comment 66

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9013695 [details] [diff] [review]
Make nsILoadURIDelegate async

Got feedback from Jim on slack, updating accordingly.

Comment 67

[Dylan Roeh (:droeh) (he/him)]

Attachment: 1.0 - Add Cycle Collection for Maybe

Comment 68

[Dylan Roeh (:droeh) (he/him)]

Attachment: 2.0 - Make nsILoadURIDelegate async

Comment 69

[Dylan Roeh (:droeh) (he/him)]

Attachment: 3.0 - Make onLinkClickSync async

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.

Comment 70

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 71

[Chris Peterson [:cpeterson]]

63=wontfix because launching app links (like Reddit's) is a low priority for Focus.

Comment 72

[Dylan Roeh (:droeh) (he/him)]

(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?

Comment 73

[Boris Zbarsky [: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.

Comment 74

[Olli Pettay [:smaug][[e-mail]]]

Hmm, submission handling... reminds me of bug 1495363, which I need to still give feedback.

Comment 75

[Anne (:annevk)]

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.

Comment 76

[Makoto Kato [:m_kato] (PTO until 5/6)]

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.

Comment 77

[Dylan Roeh (:droeh) (he/him)]

(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?

Comment 78

[Dylan Roeh (:droeh) (he/him)]

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.

Comment 79

[Dylan Roeh (:droeh) (he/him)]

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.

Comment 81

[Dylan Roeh (:droeh) (he/him)]

Attachment: Alternate approach

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.

Comment 82

[Dylan Roeh (:droeh) (he/him)]

Fixed by bug 1619798 🥳

Comment 83

[Olli Pettay [:smaug][[e-mail]]]

(Clearing the old needinfo since this got fixed elsewhere :) )