873857 - nsDocShell::LoadURI is broken, drops its postData argument

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

[env]
http://hg.mozilla.org/mozilla-central/rev/26cb30a532a1

[Step to reproduce]
1. Open Browser Console.
2. Execute the following code on JSTerm in Browser Console.
```
var url = "http://www.google.com/";
var flags = Ci.nsIWebNavigation.LOAD_FLAGS_NONE;
var post = getPostDataStream("bar", "", "", "application/x-www-form-urlencoded");
gBrowser.selectedBrowser.loadURIWithFlags(url, flags, null, null, post);
```

[Result]
Firefox requests the url with using **GET** method.
This is regression.

[Expectation]
Firefox should requests the url with using **POST** method.

xul:browser.loadURIWithFlags calls nsIWebNavigation.loadURI().
So this bug may be nsIWebNavigation.loadURI's bug.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I tested old nightly builds.
As the result of it, this build works correctly (Nightly in 2013-05-16):
http://hg.mozilla.org/mozilla-central/rev/cc139752bed4

But this build gets this bug (Nightly in 2013-05-17):
http://hg.mozilla.org/mozilla-central/rev/ea767da526ff

Comment 2

[doncodes]

This regression is breaking the StumbleUpon extension's ability to submit new sites.

Comment 3

[Josh Matthews [:jdm]]

Given http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cc139752bed4&tochange=ea767da526ff, I have a strong suspicion that bug 862401 is the cause of the regression.

Comment 4

[Josh Matthews [:jdm]]

In particular, http://hg.mozilla.org/integration/mozilla-inbound/rev/8b36d359f889#l6.79 looks like it will cause the post data stream to be nulled out even if CreateFixupURI ends up failing, which isn't the XPCOM contract that is usually followed.

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Josh Matthews [:jdm] from comment #4)
> In particular,
> http://hg.mozilla.org/integration/mozilla-inbound/rev/8b36d359f889#l6.79
> looks like it will cause the post data stream to be nulled out even if
> CreateFixupURI ends up failing, which isn't the XPCOM contract that is
> usually followed.


Your indication would be right. I traced the code flow of this case with debugger. The variable, `postStream`, pointed null on this line:
http://hg.mozilla.org/mozilla-central/file/4ac6c72b06c8/docshell/base/nsDocShell.cpp#l4040

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Josh Matthews [:jdm] from comment #4)
> In particular,
> http://hg.mozilla.org/integration/mozilla-inbound/rev/8b36d359f889#l6.79
> looks like it will cause the post data stream to be nulled out even if
> CreateFixupURI ends up failing, which isn't the XPCOM contract that is
> usually followed.

The code is relying on the assumption that if we end up calling createFixupURI, using the passed-in postData would be a bad thing. Why would createFixupURI fail, and why is it even being called in the STR from comment 0?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> The code is relying on the assumption that if we end up calling
> createFixupURI, using the passed-in postData would be a bad thing.

In particular, if createFixupURI fails, its URI outparam value is nulled out and LoadURI fails anyways.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

OK, I was a bit confused. Let's reset:
- createFixupURI doesn't touch its aPostData argument, it just passes it to KeywordToURI (sometimes via KeywordURIFixup)
- KeywordToURI will unconditionally null out its aPostData argument (and potentially replace it with postData from the search engine, if it succeeds)
- in the STR from comment 0, I don't see any reason why we'd call KeywordToURI - FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP is not passed to loadURIWithFlags

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #8)
> - in the STR from comment 0, I don't see any reason why we'd call
> KeywordToURI - FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP is not passed to
> loadURIWithFlags

Rather, LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP is not passed, which is what controls whether FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP is specified.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Turns out I don't know how getter_AddRefs works (it unconditionally clears the existing value). We only want to clobber the value if createFixupURI sets it.

Also, I'm kind of amazed we didn't have test coverage that covers this already.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 753468 [details] [diff] [review]
patch

Oh, duh.

Should we not be nulling out postStream if NS_SUCCEEDED(rv) && !fixupStream, though?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

(In reply to Boris Zbarsky (:bz) from comment #13)
> Should we not be nulling out postStream if NS_SUCCEEDED(rv) && !fixupStream,
> though?

Good question - createFixupURI doesn't always significantly alter its input, and the docshell caller can't easily tell how much it was altered (apart from being able to detect the postData-returned case). If we're calling loadURI("www.google.com", postData), and createFixupURI massages that into http://www.google.com, it's probably better to keep the postData than to clobber it, right?

In any case this patch keeps the existing behavior that loadURI had prior to bug 862401, AFAICT. I can file a followup on making this all saner if you'd like.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 753468 [details] [diff] [review]
patch

OK.  r=me.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/898466d3b437

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/898466d3b437

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 753468 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 862401
User impact if declined: lots of broken searches/add-ons
Testing completed (on m-c, etc.): on m/c, automated test
Risk to taking this patch (and alternatives if risky): very low risk
String or IDL/UUID changes made by this patch: none

Comment 20

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fbbfb571cbe2

Comment 21

[Brian Carpenter [:geeknik]]

The issue mentioned in Comment #2, the broken StumbleUpon extension, is fixed in the hourly built from http://hg.mozilla.org/mozilla-central/rev/4de82dd4b7b6. All broken functionality now works like it did before the regression. I assume it will be in tomorrow's Nightly build.