971897 - Use xpct{stubs,invoke}_x86_64 from linux/unix on OpenBSD/amd64 (no progressbar for some downloads, downloads panel shows them as failed)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Landry Breuil (:gaston)]

Since around Firefox 26, the downloads arrow shows no progressbar for some downloads, and for those the downloads panel shows them as 'Failed', even during the download (when there's a corresponding .part file). The downloads themselves are okay, but the visual feedback is most of the times wrong. I've seen it working fine in some occasions, but i havent been able to pinpoint a particular usecase where it worked/failed.

I might suspect something about os.file specifics, since it's on OpenBSD..

Comment 1

[James Turner]

I can replicate this behavior on Firefox 26 as well. I'm running OpenBSD -current on amd64. The following URL always exhibits the above behavior.

http://mirror.jmu.edu/pub/OpenBSD/snapshots/amd64/bsd.rd

Comment 2

[Landry Breuil (:gaston)]

26.0 on OpenBSD/i386 seems to works flawlessly, so maybe something about the architecture/32bits vs 64bits in os.file code dealing with file handling ?

Comment 3

[stu-mozilla-persona]

Confirmed here too with 27.0 on amd64 and i386.

Comment 4

[Fabian Raetz]

I have investigated a little more to figure out the root cause of this bug
and i made the following observations:


The root cause is that when DownloadLegacyTranfer.init(..., aIsPrivate) in 
toolkit/components/jsdownloads/src/DwonloadLegacy.js:208 is called,
the parameter "aIsPrivate" is always true.

So basically all Downloads that go through this DownloadLegacyTransfer.init(..., aIsPrivate) 
are marked as a private Download which leads to wrong behaviour in the following codepath.

This can be verified by:
 1. opening a second "private Window".
 2. download a file in either the normal or the "private" window
 3. Notice that the download is always visualized in the "private" window

If you have no "private" window open, the downloads are just not visualized / updated, which is the cause for the "Failed" state in the downloads panel.



Investigation further why DownloadLegacyTransfer.init(..., aIsPrivate) aIsPrivate parameter
is always false, i looked at the (only?) caller in 
uriloader/exthandler/nsExternalHelperAppService.cpp:2077 to see why it only pass aIsPrivate = true.

------

  rv = transfer->Init(mSourceUrl, target, EmptyString(),
                       mMimeInfo, mTimeDownloadStarted, mTempFile, this,
                       channel && NS_UsePrivateBrowsing(channel););

-----


aIsPrivate is the result of "channel && NS_UsePrivateBrowsing(channel)" which results in an correct
value of aIsPrivate = false when downloading something from a normal window and aIsPrivate = true when
downloading something from a private window. 

So all correct here , except that when the  DownloadLegacyTransfer.init(..., aIsPrivate) Method is called in C++ we have a correct aIsPrivate of either true or false but in JavaScript we receive always true.

Whats even more curious is that when i replace the "channel && NS_UsePrivateBrowsing(channel)" expression directly with "false" or store the result of the expression in a variable like below
and than passing it to the transfer->Init call, i get "sometimes" a correct result in the JavaScript code (DownloadLegacyTransfer.init(..., aIsPrivate)). 

-------

  rv = transfer->Init(mSourceUrl, target, EmptyString(),
                       mMimeInfo, mTimeDownloadStarted, mTempFile, this,
                       false);

-------
or
-------

  bool mytest = false;
  mytest = channel && NS_UsePrivateBrowsing(channel);
  rv = transfer->Init(mSourceUrl, target, EmptyString(),
                       mMimeInfo, mTimeDownloadStarted, mTempFile, this,
                       mytest);

------

With one of these changes the behaviour changed which makes no sense to me.
To me they are equal xD


So it seems that "sometimes" know dependents on the actual file size in my tests.
Small files (something < 10MB/11MB) are receiving aIsPrivate = true in DownloadLegacyTransfer.init(..., aIsPrivate), which is wrong and bigger files 
are recieving aIsPrivate = false which is corect.



My next step is to figure out why C++ sees either true or 
false and Javascript sees everytime true even if it was false i C++.

Does this look like an indicating for corrupted memory or something?


Where could i debug this further? in nsIXPConnect?


Tests are on amd64 with 30beta.

Comment 5

[Fabian Raetz]

Attachment: bug-971897-sync-xptcstubs_amd64_openbsd-with-linux_version.patch

Hi all,

the issue is in xptcall. This particular issue was fixed
in the darwin/linux/solaris version a while ago.
See https://bugzilla.mozilla.org/show_bug.cgi?id=821628


While diffing with the linux version i noticed another Bug fix is 
missing which i added here too.
Not sure where/if this causes problems though and how to test this yet.
See https://bugzilla.mozilla.org/show_bug.cgi?id=652571


We may should switch to the xptcinvoke_x86_64_unix.cpp / xptcstubs_x86_64_linux.cpp 
Versions in the future instead of keeping OpenBSD specific versions. FreeBSD uses these too, if i understand
xpcom/reflect/xptcall/md/unix/moz.build correctly.

Cheers,
Fabian

Comment 6

[Landry Breuil (:gaston)]

I see absolutely no relation between 821628 and the problem we're seeing here... care to explain ? Does your patch fixes the download panel ?

Comment 7

[Fabian Raetz]

Yes, this fixed the issue for me. 
I should have probably explained the patch to you ...

As you can see in Comment 4, i noticed that in the observed bug
only appears when the Download goes through DownloadLegacyTranfer.init(..., aIsPrivate) in
toolkit/components/jsdownloads/src/DwonloadLegacy.j:208.

The problem is that the last parameter (bool aIsPrivate) is most of the time "true" where it should be "false". So all Downloads where associated with a private Browsing session and were displayed in "private Downloads window".

The only user of DownloadLegacyTranfer.init() is in uriloader/exthandler/nsExternalHelperAppService.cpp:2077 where aIsPrivate was correct ("false" when not in private browsing mode and "true" when in private browsing mode).

So something goes wrong (with boolean values) when transitioning from native code to javascript code which is why i was looking in xpcom/. The Bug was only observed in amd64 which is why i landed in xpcom/reflect/xptcall/md/unix.

I saw the "Linux" comments in xptcstubs_amd64_openbsd.cpp which is why i made a diff to the linux version.
There were 2 chunks that changed. 

The second chunk was introduced in Bug 821628. From Bug 821628:
-----------------------------------------
I noticed that one xpcshell test was failing only on Mac, gdb confirmed the value was correct in the cpp side, but when passed to js false was becoming true.

After debugging into the darwin xpcstubs I noticed the found value was indeed not a false bool as expected.
This happens when bool is not in the first 5 integer params, since those are stored in integer registers, while from the 6th on they are passed on the stack, that is exactly my case:
void onVisit(in nsIURI aURI,
                in long long aVisitID,
                in PRTime aTime,
                in long long aSessionID,
                in long long aReferringID,
                in unsigned long aTransitionType,
                in ACString aGUID,
                in boolean aHidden);
-----------------------------------


Sounds pretty much like what we observing in our case, no? The DownloadLegacyTranfer.init (nsITransfer) function is declared as 
    void init(in nsIURI aSource,
              in nsIURI aTarget,
              in AString aDisplayName,
              in nsIMIMEInfo aMIMEInfo,
              in PRTime startTime,
              in nsIFile aTempFile,
              in nsICancelable aCancelable,
              in boolean aIsPrivate);

so aIsPrivate is put on the stack as in Bug 821628. As the patch fixed solaris/darwin/linux versions it
was kind of obvious, that we have to fix this too. And indeed this fixed the Download problem for me.

With the fix from Bug 821628 applied, only one line was left were xptcstubs_amd64_openbsd.cpp differs from xptcstubs_x86_64_linux.cpp which was introduced in Bug 652571 and was applied to solaris/darwin/linux too.
So i thought it is a good idea to sync this fix with our xptcstubs_amd64_openbsd.cpp version while here.


Not sure if this makes sense to you ... let me know if i should start a second attempt, explaining it tommorow :)

gn8,
Fabian

Comment 8

[Landry Breuil (:gaston)]

Comment on attachment 8508881 [details] [diff] [review]
bug-971897-sync-xptcstubs_amd64_openbsd-with-linux_version.patch

Thanks for the detailed explanation (and the time spent digging into this!), that makes sense to me now - i assume you've been running with this for a while, and found no regression ?

Benjamin, what do you think about it ? Should we fix xptcstubs_amd64_openbsd.cpp or rather use the linux version for consistency with FreeBSD, since they also use the same ?

Comment 9

[Fabian Raetz]

I've been running with this change for just a view hours - no regressions spotted so far.

Do you plan to send a second mozilla-firefox-34 beta package to [e-mail]?
Maybe it would be a good idea to integrate the patch there, so we get a few more tests.

Comment 10

[Benjamin Smedberg]

It doesn't matter greatly to me, but if it's simple to use the shared Linux version, I always like removing duplicate code in general.

Comment 11

[Landry Breuil (:gaston)]

What confuses me, looking at the moz.build file, if we decide to remove the block lines 51->55 and add 'OpenBSD' to the list in http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptcall/md/unix/moz.build#29, then maybe we should remove 'OpenBSD' from the previous block line 22.

elif CONFIG['OS_TEST'].find('86') != -1:
if CONFIG['CPU_ARCH'] == 'x86':

this syntax really puzzles me.. and i want to be sure it's not going to break more say alpha/sparc64/powerpc/etc...... does this look sane ?

Comment 12

[Landry Breuil (:gaston)]

Attachment: bug-971897-use_x86_64_xptcstubs_xptcinvoke_from_linux

And here's a bigger hammer doing what i said in the previous comment in moz.build. I've built this on amd64 within 34.0b2 and ran it without issues for a day.

I didnt realize until now that this patch was *also* syncing xptcinvoke, not only xptcstubs - looking at the differences, between xptcinvoke_amd64_openbsd.cpp (which hasnt really been touched in a while) and xptcinvoke_x86_64_unix.cpp that also brings us fixes from bugs 776079 (http://hg.mozilla.org/mozilla-central/rev/727721e5d8ac), 783523 (http://hg.mozilla.org/mozilla-central/diff/844b142d8111/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp) and 623129 (http://hg.mozilla.org/mozilla-central/rev/236989b3a807).

We already use the linux/unix versions on OpenBSD/i386, so that'll be also for consistency.

(funny comment of the day: at the top of xptcstubs_x86_64_{linux,darwin}.cpp it is said to keep linux and darwin versions in sync.. apparently, that failed :)

Comment 13

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb1566abd2c pushed att 8510841, been testing it w/ 34.0b2/b3 since a while without issues. Thanks fabian for finding the actual source of the issue!

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3bb1566abd2c