1209591 - Address bar reverts to previous page's URL when entering an invalid URI with keyword.enabled=false

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[dagger.bugzilla]

STR:
1) Set keyword.enabled = false
2) Load, say, https://hg.mozilla.org/ in a new tab
3) Type "2600::" into the address bar and hit enter

Expected result: "The address isn't valid" error page with "2600::" in the address bar. [1]
Actual result: "The address isn't valid" error page with "https://hg.mozilla.org/" in the address bar.

The regression range is:
  https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=001942e461&tochange=e9aea2b7d9
so I guess it was regressed by bug 1189082 [2].


[1] Actually, what I'd really expect is that typing an IP directly into the address bar would work properly, but that's a separate bug...
[2] Diff: http://hg.mozilla.org/mozilla-central/rev/5ee6279df679

Comment 1

[dagger.bugzilla]

(I can't add bug 1189082 to the dependences myself, so I'm CCing the author of its patch instead.)

Comment 2

[:Gijs (he/him)]

(In reply to dagger.bugzilla from comment #0)
> [1] Actually, what I'd really expect is that typing an IP directly into the
> address bar would work properly, but that's a separate bug...

You want [2600::] .

Anyway, this needs to be sorted out. I don't understand why docshell is both displaying the error page and returning an error to us - they should be picking one or the other (or always pick both). It sounds like now we can't properly distinguish cases where the document loaded does and doesn't change, which is stupid. :-\

Comment 3

[:Gijs (he/him)]

Boris, sorry to bug you here, but can you clarify how callers are supposed to distinguish the cases where we load an error page and where we do not? It seems like the resultcodes are the same (for this case and the one in 1189082) - and I could try changing that for the case from bug 1189082, but I'm going to have some job trying to find an rv that doesn't overlap with anything else...

Comment 4

[Boris Zbarsky [:bzbarsky]]

The error page story sucks.  They don't send any of the normal loading notifications.... :(

I don't know how you can distinguish offhand.  Maybe Olli does?

Comment 5

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

I don't know. Would need to go through the code.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

If I read this correctly, the problem is that loadURIWithOptions flattens everything to NS_ERROR_FAILURE.

We could just return rv after DisplayLoadError call. That would return NS_ERROR_MALFORMED_URI instead of flatten it to NS_ERROR_FAILURE. From the outside we can detect that and react accordingly. There should be no problem with existing consumers since none of them can care about checking the error is the generic NS_ERROR_FAILURE.

Otherwise we could introduce a new error like NS_ERROR_DOCSHELL_ERRORPAGE and return it everywhere DisplayLoadError is invoked and the code would bailout with an error. But that's sort of abusing the error to give a hint.

Comment 7

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #6)
> If I read this correctly, the problem is that loadURIWithOptions flattens
> everything to NS_ERROR_FAILURE.
> 
> We could just return rv after DisplayLoadError call. That would return
> NS_ERROR_MALFORMED_URI instead of flatten it to NS_ERROR_FAILURE. From the
> outside we can detect that and react accordingly. There should be no problem
> with existing consumers since none of them can care about checking the error
> is the generic NS_ERROR_FAILURE.

So what you're suggesting is forwarding the real error instead of the flattening after DisplayLoadError, and then only reverting the URL bar for NS_ERROR_FAILURE instead of for any exceptions ? If so, that sounds good to me - it seems the flattening code has been there since forever, dunno if/what would break if we don't flatten/normalize... but probably worth a trypush.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Gijs Kruitbosch from comment #7)
> dunno if/what would break if we don't flatten/normalize...

From a logical point of view, it's very unlikely someone is actually checking for NS_ERROR_FAILURE since everything is flattened and it's a so generic error (the most generic we have iirc). it's more likely current consumers are catching any exception. For those consumers nothing will change.

Comment 9

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
We need to not ship this regression.

Comment 10

[:Gijs (he/him)]

Attachment: don't break location bar for users with keyword.enabled = false when they enter broken URLs,

I think this should take care of both issues in the blocked bug, while no longer breaking the malformed URI case for keyword.enabled. Marco and Olli, does this look sane?

Comment 11

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef56d89c0991

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8677533 [details] [diff] [review]
don't break location bar for users with keyword.enabled = false when they enter broken URLs,

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

::: docshell/base/nsDocShell.cpp
@@ +4649,5 @@
>    }
>  
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }

Is it really useful to expose all the other errors that are not NS_ERROR_MALFORMED_URI?
you could just return rv inside the previous if.

Note, I'm not against more fine-grained error exposure, but I don't think, as it is, this works as expected. In urlbarBindings you are doing:

if (ex.result == Cr.NS_ERROR_FAILURE) {
  this.handleRevert();
} else {
  Cu.reportError(ex);
}

This would work if all the errors BUT NS_ERROR_MALFORMED_URI would be flattened to NS_ERROR_FAILURE.
So either you still flatten everything but NS_ERROR_MALFORMED_URI, or change the if as
if (ex.result != NS_ERROR_MALFORMED_URI)

@@ +4762,5 @@
>      error.AssignLiteral("unknownProtocolFound");
> +  } else if (NS_ERROR_FILE_NOT_FOUND == aError ||
> +             (NS_ERROR_NOT_AVAILABLE == aError && aURI &&
> +              NS_SUCCEEDED(aURI->SchemeIs("resource", &isResourceURI)) &&
> +              isResourceURI)) {

Is this a fix for another issue unrelated to this report?
I'm not sure, but I suspect this problem is not just limited to resource uris... don't file uris act the same? (I suspect most protocols return NS_ERROR_NOT_AVAILABLE)
Maybe NS_ERROR_NOT_AVAILABLE should be generally assigned to fileNotFound errors or should have a resourceNotAvailable kind of error?
Sounds like this simple fix may open a can of worms...

Comment 13

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #12)
> Comment on attachment 8677533 [details] [diff] [review]
> don't break location bar for users with keyword.enabled = false when they
> enter broken URLs,
> 
> Review of attachment 8677533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +4649,5 @@
> >    }
> >  
> > +  if (NS_FAILED(rv)) {
> > +    return rv;
> > +  }
> 
> Is it really useful to expose all the other errors that are not
> NS_ERROR_MALFORMED_URI?
> you could just return rv inside the previous if.
> 
> Note, I'm not against more fine-grained error exposure, but I don't think,
> as it is, this works as expected. In urlbarBindings you are doing:
> 
> if (ex.result == Cr.NS_ERROR_FAILURE) {
>   this.handleRevert();
> } else {
>   Cu.reportError(ex);
> }
> 
> This would work if all the errors BUT NS_ERROR_MALFORMED_URI would be
> flattened to NS_ERROR_FAILURE.
> So either you still flatten everything but NS_ERROR_MALFORMED_URI, or change
> the if as
> if (ex.result != NS_ERROR_MALFORMED_URI)

This doesn't really make sense. Many other errors also display error pages. Really, what we want to know is "Did the page displayed to the user change". But we don't get that information from the loadURI call, so we're trying to infer it from the error code.

It's fragile however we do it. I don't really have great ideas about how to improve this - we could in principle cause the error page load to fire a specific event or handler, maybe? But the problem is that really what we want is to know when the error page *doesn't* load.

> @@ +4762,5 @@
> >      error.AssignLiteral("unknownProtocolFound");
> > +  } else if (NS_ERROR_FILE_NOT_FOUND == aError ||
> > +             (NS_ERROR_NOT_AVAILABLE == aError && aURI &&
> > +              NS_SUCCEEDED(aURI->SchemeIs("resource", &isResourceURI)) &&
> > +              isResourceURI)) {
> 
> Is this a fix for another issue unrelated to this report?
> I'm not sure, but I suspect this problem is not just limited to resource
> uris... don't file uris act the same? (I suspect most protocols return
> NS_ERROR_NOT_AVAILABLE)

No. file:/// returns NS_ERROR_FILE_NOT_FOUND, that's already there in the if statement...

The reason for doing this is bug 1189082 comment 18.

It may be necessary to audit other places.

> Maybe NS_ERROR_NOT_AVAILABLE should be generally assigned to fileNotFound
> errors or should have a resourceNotAvailable kind of error?

Maybe. I talked with Olli about this yesterday night. I'm hopeful he can suggest an alternative.

> Sounds like this simple fix may open a can of worms...

It already has, yeah. Though to be honest, if the outcome is a saner story for our error page loads, that sounds good to me.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Gijs Kruitbosch from comment #13)
> This doesn't really make sense. Many other errors also display error pages.

Yes, and many other errors don't...
In the first part of LoadURIWithOptions, the only error that can display an error page is NS_ERROR_MALFORMED_URI, any error before that point should then still be flattened so from the outside it's "clear" nothing loaded. Instead your are pushing out other errors with that return rv and then from the outside we again have no idea if the error page was shown or not.
If the idea is "NS_ERROR_FAILURE means no error page was loaded, other errors implicate an error page" then anything not loading an error page must be flattened. Not doing that we'd just introduce more uncertainty.

From the NS_ERROR_MALFORMED_URI point on, it's all crazy, again if the idea is to distinguish through error codes, we should flatten also the CreateLoadInfo error, and we should do something about LoadURI, here becomes the huge pain, cause LoadURI can return anything... but at least we know the load process somehow started...

> It's fragile however we do it.

Yes, relying on error results is extremely fragile.

I don't really have great ideas about how to
> improve this - we could in principle cause the error page load to fire a
> specific event or handler, maybe? But the problem is that really what we
> want is to know when the error page *doesn't* load.

Maybe we should just do this at the ui level, checking if an error page loads... But would still be fragile, cause we need to do something if the error page does NOT load. That involves either a timer (very fragile handling) or catching a custom event fired when the error page load.
The custom even may be the best path forward, it may still be fancy to handle the right order of events with e10s...

Comment 15

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

Comment on attachment 8677533 [details] [diff] [review]
don't break location bar for users with keyword.enabled = false when they enter broken URLs,

So I don't think we should take this since I have no idea why only "resource" gets that special handling.
Could we not fix resource protocol handler to trigger the right error?
Adding more and more mysterious special cases isn't the way to go here.

Comment 16

[:Gijs (he/him)]

Attachment: allow loadURI consumers to expose whether an error page was immediately loaded as result of an error,

Would this be better? I believe that these are the only DisplayLoadError calls that are in this codepath, but I could be wrong. I made this opt-in with a flag because I don't think all loads will want this behaviour.

Comment 17

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

Comment on attachment 8679500 [details] [diff] [review]
allow loadURI consumers to expose whether an error page was immediately loaded as result of an error,

So almost like this but DisplayLoadError returning success doesn't mean error was shown since  if (error.IsEmpty()) { return NS_OK; } in it.
Could we make DisplayLoadError (in .idl) to return bool indicating whether error page was shown?

Comment 18

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1209591 - allow loadURI consumers to expose whether an error page was immediately loaded as result of an error, r?smaug,mak

Bug 1209591 - allow loadURI consumers to expose whether an error page was immediately loaded as result of an error, r?smaug,mak

Comment 19

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #17)
> Comment on attachment 8679500 [details] [diff] [review]
> allow loadURI consumers to expose whether an error page was immediately
> loaded as result of an error,
> 
> So almost like this but DisplayLoadError returning success doesn't mean
> error was shown since  if (error.IsEmpty()) { return NS_OK; } in it.
> Could we make DisplayLoadError (in .idl) to return bool indicating whether
> error page was shown?

I think I've done this, but all the unused bools feel a bit dumb. Is there a better way to do this?

Comment 20

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

.idl isn't too friendly here.

one could add
%{C++
  bool DisplayLoadError(nsresult aError, nsIURI* aURI,
                        const char16_t* aURL,
                        nsIChannel* aFailedChannel)
  {
    bool didDisplayLoadError = false;
    DisplayLoadError(aError, aURI, aURL, aFailedChannel, &didDisplayLoadError);
    return didDisplayLoadError;
  }
%}

to nsIDocShell.idl

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8680597 [details]
MozReview Request: Bug 1209591 - allow loadURI consumers to expose whether an error page was immediately loaded as result of an error, r?smaug,mak

https://reviewboard.mozilla.org/r/23647/#review21273

The flag thing looks like a lot of complication to me, it's easy to miss, plus it hides the real exception reason.

A better alternative off-hand may be to modify loadURIWithOptions providing an optional inout param like didShowErrorPage. The problem there is that we'd need to modify also LoadURI and InternalLoad. Fixing their consumers wouldn't be too complicate cause it's all cpp and the compiler can help. But this is more work.
If smaug thinks the current approach is better I won't block on that.

The browser changes look correct.

Comment 22

[:Gijs (he/him)]

Comment on attachment 8680597 [details]
MozReview Request: Bug 1209591 - allow loadURI consumers to expose whether an error page was immediately loaded as result of an error, r?smaug,mak

Bug 1209591 - allow loadURI consumers to expose whether an error page was immediately loaded as result of an error, r?smaug,mak

Comment 23

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1209591 - stop using result of displayLoadError on android, r?margaret

Bug 1209591 - stop using result of displayLoadError on android, r?margaret

Comment 24

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #23)
> Created attachment 8682480 [details]
> MozReview Request: Bug 1209591 - stop using result of displayLoadError on
> android, r?margaret
> 
> Bug 1209591 - stop using result of displayLoadError on android, r?margaret

So this change is because somehow it seems android expects displayLoadError to return a request, which AFAICT it will never ever do (either before or after this patch), so I just ended up removing the code. Margaret, can you sanity-check that?

Comment 25

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #20)
> .idl isn't too friendly here.
> 
> one could add
> %{C++
>   bool DisplayLoadError(nsresult aError, nsIURI* aURI,
>                         const char16_t* aURL,
>                         nsIChannel* aFailedChannel)
>   {
>     bool didDisplayLoadError = false;
>     DisplayLoadError(aError, aURI, aURL, aFailedChannel,
> &didDisplayLoadError);
>     return didDisplayLoadError;
>   }
> %}
> 
> to nsIDocShell.idl

I couldn't get this to work, so I stuck it in the header file instead.

Because of that and comment #21, re-requesting review.

Comment 26

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

(In reply to Marco Bonardo [::mak] from comment #21)
> A better alternative off-hand may be to modify loadURIWithOptions providing
> an optional inout param like didShowErrorPage. 
inouts tend to be very error prone, and I would avoid them unless there is some really good reason for them.


> If smaug thinks the current approach is better I won't block on that.
I don't quite understand what setup you're proposing

Comment 27

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

(In reply to Olli Pettay [:smaug] from comment #26) 
> > If smaug thinks the current approach is better I won't block on that.
> I don't quite understand what setup you're proposing
Ah, maybe I do see what you're proposing. Not sure that is any better. Do we need the actually error information in the caller?

Comment 28

[:Margaret Leibovic]

Comment on attachment 8682480 [details]
MozReview Request: Bug 1209591 - stop using result of displayLoadError on android, r?margaret

https://reviewboard.mozilla.org/r/24073/#review21585

::: mobile/android/chrome/content/browser.js:4505
(Diff revision 1)
> -      aRequest = this.browser.docShell.displayLoadError(Cr.NS_ERROR_UNKNOWN_PROTOCOL, fixedURI, null);
> +      this.browser.docShell.displayLoadError(Cr.NS_ERROR_UNKNOWN_PROTOCOL, fixedURI, null);

I didn't look closely at the previous patch... does this method now change the value of fixedURI? (Or did it do that before but the if statement below wasn't necessary?) I don't have time to make a build to test right now, but assuming this maintains the same behavior as before, this seems fine.

Comment 29

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Olli Pettay [:smaug] from comment #27)
> Ah, maybe I do see what you're proposing. Not sure that is any better. Do we
> need the actually error information in the caller?

If we never need the actual error information, why having a flag that states the caller doesn't care? The flag is clearly there cause we are afraid to break some consumer, but it is just a workaround hack.

What I suggested was adding "[optional] out boolean didShowErrorPage" and then use didShowErrorPage.value.
I think I confused you with "inout", sorry.

Comment 30

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

I could live with that too, unless it makes the code in Docshell significantly more complicated.

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/34c0df339934

Comment 32

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #30)
> I could live with that too, unless it makes the code in Docshell
> significantly more complicated.

I'll look at this tomorrow.

(In reply to :Margaret Leibovic from comment #28)
> Comment on attachment 8682480 [details]
> MozReview Request: Bug 1209591 - stop using result of displayLoadError on
> android, r?margaret
> 
> https://reviewboard.mozilla.org/r/24073/#review21585
> 
> ::: mobile/android/chrome/content/browser.js:4505
> (Diff revision 1)
> > -      aRequest = this.browser.docShell.displayLoadError(Cr.NS_ERROR_UNKNOWN_PROTOCOL, fixedURI, null);
> > +      this.browser.docShell.displayLoadError(Cr.NS_ERROR_UNKNOWN_PROTOCOL, fixedURI, null);
> 
> I didn't look closely at the previous patch... does this method now change
> the value of fixedURI?

No, it does not do this.

> (Or did it do that before but the if statement below
> wasn't necessary?)

No. the result of calling docShell.displayLoadError was always undefined; there's no out param defined. But the other patch will change this. Because of the falsy-check for the result which we're overwriting here, this never did anything (aRequest doesn't get reused in this method), it was just pointless.

> I don't have time to make a build to test right now, but
> assuming this maintains the same behavior as before, this seems fine.

Right. You may or may not want to review why the code was put here in the first place and what it was *supposed* to do that it either never did or stopped doing at some point...

https://hg.mozilla.org/integration/fx-team/rev/34c0df339934

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34c0df339934

Comment 34

[:Gijs (he/him)]

Attachment: part 1: make XPIDL set default nullptrs for optional out parameters when there's no retval,

As discussed on IRC. I can't easily test the AString/DOMString case because there aren't any uses in tree... but this seems to work and compiles on my local machine, at least... it's probably worth a try push before landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14d0ed5c2940

Comment 35

[:Gijs (he/him)]

Attachment: part 2: pass around our error page display stuff so frontend can use it,

That makes this comparably simple... though still annoying how many things implement nsIWebNavigation. Oh well.

Comment 36

[:Gijs (he/him)]

Comment on attachment 8683903 [details] [diff] [review]
part 1: make XPIDL set default nullptrs for optional out parameters when there's no retval,

bzexport hates me today.

Comment 37

[:Gijs (he/him)]

Attachment: part 3: use the new API things in the frontend

Comment 38

[:Gijs (he/him)]

Comment on attachment 8683903 [details] [diff] [review]
part 1: make XPIDL set default nullptrs for optional out parameters when there's no retval,

Neil voiced skepticism on IRC about this being a good idea, so going to ping him too, I guess.

Comment 39

[:Gijs (he/him)]

Comment on attachment 8683905 [details] [diff] [review]
part 2: pass around our error page display stuff so frontend can use it,

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

::: docshell/base/nsDocShell.h
@@ -711,5 @@
>    bool DoAppRedirectIfNeeded(nsIURI* aURI,
>                               nsIDocShellLoadInfo* aLoadInfo,
>                               bool aFirstParty);
>  
> -protected:

There's another protected higher up, and nothing inbetween, and I thought I needed to touch this - turned out I didn't, but this got left in. I can take it out if you prefer.

Comment 40

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8683905 [details] [diff] [review]
part 2: pass around our error page display stuff so frontend can use it,

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

just a couple drive-by

::: docshell/base/nsDocShell.cpp
@@ +1570,5 @@
>                        sourceDocShell,
>                        baseURI,
>                        nullptr,  // No nsIDocShell
> +                      nullptr,
> +                      aDisplayedErrorPage); // No nsIRequest

the comment has been moved to the wrong line

::: docshell/base/nsIDocShell.idl
@@ +475,5 @@
>    void displayLoadError(in nsresult aError,
>                          in nsIURI aURI,
>                          in wstring aURL,
> +                        [optional] in nsIChannel aFailedChannel,
> +                        [optional] out boolean aDisplayedPage);

considered the API scope (showing the error page), I'd say it could just return a boolean instead of having an out param defined like this.

Comment 41

[[e-mail]]

Comment on attachment 8683903 [details] [diff] [review]
part 1: make XPIDL set default nullptrs for optional out parameters when there's no retval,

I was concerned by the mechanics of default arguments on virtual methods but as it happens it's roughly equivalent to defining a non-virtual overload. You don't get to see the default argument if you already have a concrete instance though, since the virtual override shadows it.

>+        while (paramIter >= 0 and m.params[paramIter].optional and
>+                m.params[paramIter].paramtype == "out"):
>+            t = m.params[paramIter].type
>+            if t == "AString" or t == "DOMString":
>+                l[paramIter] += " = EmptyString()"
XPIDL doesn't actually have string out parameters; instead all string parameters are references, so they can't default to null, and out strings are non-const, so they can't default to EmptyString() either.

Comment 42

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #40)
> ::: docshell/base/nsIDocShell.idl
> @@ +475,5 @@
> >    void displayLoadError(in nsresult aError,
> >                          in nsIURI aURI,
> >                          in wstring aURL,
> > +                        [optional] in nsIChannel aFailedChannel,
> > +                        [optional] out boolean aDisplayedPage);
> 
> considered the API scope (showing the error page), I'd say it could just
> return a boolean instead of having an out param defined like this.

That would mean I'd have to write another alias that doesn't take an extra param, though, or upgrade all the c++ callers. This way causes fewer changes, though I can reintegrate the header alias that the previous patch has and make this a return value if people feel strongly. It doesn't affect the frontend patch.

Comment 43

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Gijs Kruitbosch from comment #42)
> That would mean I'd have to write another alias that doesn't take an extra
> param, though, or upgrade all the c++ callers. 

Wouldn't the method signature be the same in cpp? note: I didn't look at your changes to xpidl, did you make so that cpp methods can avoid optional params?
fwiw there are only 7 calls of DisplayLoadError, all in nsDocshell.cpp.
Btw, it will work regardless so feel free to ignore me, if that makes things easier.

Comment 44

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #43)
> (In reply to :Gijs Kruitbosch from comment #42)
> > That would mean I'd have to write another alias that doesn't take an extra
> > param, though, or upgrade all the c++ callers. 
> 
> Wouldn't the method signature be the same in cpp? note: I didn't look at
> your changes to xpidl, did you make so that cpp methods can avoid optional
> params?

Yes. This was bz's suggestion; it simplified the changes to loaduri, loaduriwithoptions and internalload - now all the existing callers Just Work.

> Btw, it will work regardless so feel free to ignore me, if that makes things
> easier.

Oh, I agree it's a reasonable suggestion. IMHO it mostly depends whether we prefer "cleaner" IDL or "cleaner" C++ here. I'll let Olli decide.

Comment 45

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

Not sure what the comparison between "cleaner" IDL and "cleaner" C++ is here, since if we can
have default values for optional params also in C++, not only JS, that just makes the setup
everywhere more consistent, IMO.
So, I like the xpidl change, though, wonder whether it compiles with different types without compiler warnings. Can one really assign nullptr to, say double, without a warning?

Comment 46

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #45)
> Not sure what the comparison between "cleaner" IDL and "cleaner" C++ is
> here, since if we can
> have default values for optional params also in C++, not only JS, that just
> makes the setup
> everywhere more consistent, IMO.
> So, I like the xpidl change, though, wonder whether it compiles with
> different types without compiler warnings. Can one really assign nullptr to,
> say double, without a warning?

A double out param would be double*, right, and then nullptr would be valid, I think?

Comment 47

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

silly me :)

Comment 48

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

(I obviously had in mind the setup where we would have default value also for optional 'in' params. That would make the setup consistent with js callers.)

Comment 49

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #45)
> Not sure what the comparison between "cleaner" IDL and "cleaner" C++ is
> here, since if we can
> have default values for optional params also in C++, not only JS, that just
> makes the setup
> everywhere more consistent, IMO.

I think the difference is that this IDL:

bool foo()

can be called in JS as:

obj.foo()
// (returns the bool, which we can assign into something or drop on floor)

and in C++ as

bool* unusedRV = false;
Foo(unusedRV);

whereas this IDL:

void foo([optional] out rv);

can be called in JS as:

obj.foo()
// (returns void)
OR
let x = {};
obj.foo(x);
// now x.value is the (optional) return value


and in C++ as:

Foo() // with the xpidl patch
or
bool* rvPtr = false;
Foo(rvPtr);

So the optional syntax now has simpler C++ but clumsier JS, whereas the return value has simpler JS but means you have to update all the callers. Changing the C++ semantics of the retval here to also be optional seems to me to probably more breakage-prone than what the existing xpidl patch does. Likewise, changing the JS semantics of a single optional out param seems like a recipe for breaking lots of consumers...

Am I misunderstanding what you're saying or missing a trick? I can look into updating the C++ rendering of a retval to be optional with the xpidl header generation, but I worry about breakage, and the fact that this all needs to go into 44 :-(

Comment 50

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #48)
> (I obviously had in mind the setup where we would have default value also
> for optional 'in' params. That would make the setup consistent with js
> callers.)

Right. We can look at doing something clever-er for the in-param version, but that would be more work, and isn't necessary for this patch. I can file a followup if you like.

Comment 51

[Boris Zbarsky [:bzbarsky]]

> So the optional syntax now has simpler C++ but clumsier JS

What happens with:

  void foo([optional, retval] out rv);

?  (Yes, the _IDL_ is insane, but I expect the JS and C++ would look nice if it works as I think it should.)

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8683903 [details] [diff] [review]
part 1: make XPIDL set default nullptrs for optional out parameters when there's no retval,

Now that Neil points it out, he's right about the string situation, of course.

I think we should just either skip over AString/DOMString/ACString/AUTF8String params and do nothing or throw an exception for those.

r=me with that.

Comment 53

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

Comment on attachment 8683905 [details] [diff] [review]
part 2: pass around our error page display stuff so frontend can use it,

I wouldn't make InternalLoad's last param optional. Otherwise it is easy to forget to pass the param around, like in this patch if load is targeted to some other docshell, targetDocShell->InternalLoad doesn't get the last param passed, as it should.

Sorry that this is so complicated and error prone.

Also, this needs uuids updated.

Comment 54

[:Gijs (he/him)]

So this totally doesn't actually work. Because outparams don't get converted for JS use by xpcom on non-success return values (thanks, Ms2ger, and https://dxr.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b/js/xpconnect/src/XPCWrappedNative.cpp#1414-1425 ).

Changing that probably breaks a whole bunch of stuff and add-ons and so on, and I'm tired of fighting ancillary battles/code here, so I'm going to land the already-r+'d patch that I obsoleted earlier today instead. I'll spin the optional xpcom thing off to another bug and land it anyway, because that does seem more generally useful in that it allows for neat xpcom C++/JS calls - it just happens to not work very well when you return a non-success rv.

Comment 55

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e32f4d18e3d

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04fc84c9efd2

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04fc84c9efd2

Comment 58

[:Gijs (he/him)]

Comment on attachment 8680597 [details]
MozReview Request: Bug 1209591 - allow loadURI consumers to expose whether an error page was immediately loaded as result of an error, r?smaug,mak

Approval Request Comment
[Feature/regressing bug #]: bug 1189082
[User impact if declined]: location bar behaves incorrectly with ipv6 URIs if keyword.enabled is set to false
[Describe test coverage new/current, TreeHerder]: no tests specifically for this issue
[Risks and why]: low-ish. Has been on nightly for a while now with no reported regressions. We do have reasonable general automated test coverage of behaviour of docshell and the url bar, which also hasn't detected any issues with the patch.
[String/UUID change made/needed]: yes, there is a UUID change in this patch. I believe that is OK for aurora.

NB: Both changesets associated with this bug, ie this and the android patch, will  need uplift.

Comment 59

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

Tracked as this looks like a recent regression in 44.

Comment 60

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

dagger, could you please verify that this issue is fixed as expected on the latest nightly build? Thanks!

Comment 61

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

Comment on attachment 8680597 [details]
MozReview Request: Bug 1209591 - allow loadURI consumers to expose whether an error page was immediately loaded as result of an error, r?smaug,mak

Given that this was a recent regression in 44, we should take this fix. The patch (desktop one) seems pretty large but it has baked on Nightly for a week so it seems safe to uplift to Aurora44.

Comment 62

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

Comment on attachment 8682480 [details]
MozReview Request: Bug 1209591 - stop using result of displayLoadError on android, r?margaret

Fennec patch, Aurora44+

Comment 63

[Petruta Horea [:phorea], Desktop QA]

Tested with Nightly 44.0a1 2015-09-29 and 45.0a1 2015-11-18 under Win 7 64-bit and Mac OS X 10.11.
Please see my results below:

- affected build: e10s window - "The address isn't valid" & the location bar is empty
                  non e10s window - "The address isn't valid" & previously loaded url
- fixed build: e10s window - "The address isn't valid" & the location bar is empty
               non e10s window - "The address isn't valid" & "2600::" in urlbar  

So I could only reproduce and verify this is fixed in non e10s windows.

Shouldn't we see the same entered ipv6 url in e10s windows as well?

Comment 64

[:Gijs (he/him)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #63)
> Tested with Nightly 44.0a1 2015-09-29 and 45.0a1 2015-11-18 under Win 7
> 64-bit and Mac OS X 10.11.
> Please see my results below:
> 
> - affected build: e10s window - "The address isn't valid" & the location bar
> is empty
>                   non e10s window - "The address isn't valid" & previously
> loaded url
> - fixed build: e10s window - "The address isn't valid" & the location bar is
> empty
>                non e10s window - "The address isn't valid" & "2600::" in
> urlbar  
> 
> So I could only reproduce and verify this is fixed in non e10s windows.
> 
> Shouldn't we see the same entered ipv6 url in e10s windows as well?

That is likely related to bug 1217387. This fix was for non-e10s. I'm not sure the change for bug 1189082 actually changed anything for ipv6 entry while in e10s.

Comment 65

[Petruta Horea [:phorea], Desktop QA]

Thanks, Gijs!

I don't have access to those bugs, but since this one is related to non-e10s, I'm marking it as verfied for 45.

Comment 66

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/97c16a9b54de
https://hg.mozilla.org/releases/mozilla-aurora/rev/5dc9c9025ad7

Comment 67

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/97c16a9b54de
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5dc9c9025ad7

Comment 68

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Aurora 44.0a1 2015-11-20 in non-e10s windows under Win 7 64-bit and Mac OS X 10.8.5.

Comment 69

[dagger.bugzilla]

Looks fixed for me too, thanks. I noticed another bug with loading these pages while testing (they don't seem to go into tab history properly if e10s is off; filed as bug 1226781), but since the regression range for that is a year ago it's unrelated to this one.