1720360 - DownloadSource.toSerializable does not always return an accurate representation

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect, P3)

Description

[Rob Wu [:robwu]]

See https://searchfox.org/mozilla-central/rev/aa67c5a592026cf42e15b02371259d808d086eb3/toolkit/components/downloads/DownloadCore.jsm#1425-1428 :

    // Simplify the representation if we don't have other details.
    if (!this.isPrivate && !this.referrerInfo && !this._unknownProperties) {
      return this.url;
    }

This early return was implemented correctly when it was introduced, but since then properties have been added beyond what's specified: loadingPrincipal (bug 1579911), cookieJarSettings (bug 1691907).

To fix this, it would probably make more sense to remove the condition, and replace it with the following at the end:

    // Simplify the representation if we don't have other details.
    if (Object.keys(serializable).length === 1) {
      return this.url;
    }
    return serializable;

Comment 1

[Marco Bonardo [:mak]]

Makes sense, would you like to make a patch for it?

do you know if this is causing some of the existing bugs?

Comment 2

[Rob Wu [:robwu]]

(In reply to Marco Bonardo [:mak] from comment #1)

Makes sense, would you like to make a patch for it?

I can easily submit a patch with the fix (as I described it in the report), but don't have time to develop unit tests.

do you know if this is causing some of the existing bugs?

Well the referenced code affects the branches of loadingPrincipal and cookieJarSettings, so this bug would cause bug 1691907 and bug 1579911 after a browser restart. In bug 1669566, userContextId is being added, so this bug could result in downloads being associated with the wrong container tab after a browser restart.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Rob Wu [:robwu] from comment #2)

(In reply to Marco Bonardo [:mak] from comment #1)

Makes sense, would you like to make a patch for it?

I can easily submit a patch with the fix (as I described it in the report), but don't have time to develop unit tests.

for the most part this is a code refactoring to make it more robust, thus I don't mind that much.

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1720360 - Improve correctness of DownloadSource.toSerializable

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d2dff6bcfee
Improve correctness of DownloadSource.toSerializable r=mak

Comment 6

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/0d2dff6bcfee