Closed Bug 873857 Opened 7 years ago Closed 7 years ago

nsDocShell::LoadURI is broken, drops its postData argument

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox23 + fixed
firefox24 + fixed

People

(Reporter: tetsuharu, Assigned: Gavin)

References

Details

(Keywords: regression)

Attachments

(1 file)

[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.
Keywords: regression
Hardware: x86 → x86_64
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
This regression is breaking the StumbleUpon extension's ability to submit new sites.
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.
Blocks: 862401
(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
(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?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
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
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Assignee: nobody → gavin.sharp
Attached patch patchSplinter Review
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.
Attachment #753468 - Flags: review?
Attachment #753468 - Flags: review? → review?(bzbarsky)
Component: XUL Widgets → Document Navigation
OS: Mac OS X → All
Product: Toolkit → Core
Hardware: x86_64 → All
Summary: xul:browser.loadURIWithFlags() doesn't work with passing Postdata → nsDocShell::LoadURI is broken, drops its postData argument
No longer blocks: 874870
Duplicate of this bug: 874870
Duplicate of this bug: 875223
Comment on attachment 753468 [details] [diff] [review]
patch

Oh, duh.

Should we not be nulling out postStream if NS_SUCCEEDED(rv) && !fixupStream, though?
(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 on attachment 753468 [details] [diff] [review]
patch

OK.  r=me.
Attachment #753468 - Flags: review?(bzbarsky) → review+
Duplicate of this bug: 875182
https://hg.mozilla.org/mozilla-central/rev/898466d3b437
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #753468 - Flags: approval-mozilla-aurora?
Attachment #753468 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
You need to log in before you can comment on or make changes to this bug.