1668952 - Can't upload photos from the photo gallery on ebay.com

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Ksenia Berezina [:ksenia]]

Attachment: 58769.html

This was initially reported in https://github.com/webcompat/web-bugs/issues/58769

Steps to reproduce (this requires an account on ebay.com):

1. Visit https://bulksell.ebay.com/ws/eBayISAPI.dll?SingleList in Firefox Nightly 201001 on Android and sign in
2. Click on "Create a template" button
3. Scroll down to the Photos section and click Add photos
4. Select a photo taken with a camera from a photo gallery

Expected:
Photo uploaded
Actual:
Photo doesn't upload, the upload process remains in loading state

I've attached a reduced test case. The problem seems to be that in Firefox on Android img.onload never gets called, neither does img.onerror with large data URIs. The image I'm using is 3.7MB in size.

This is probably related to https://bugzilla.mozilla.org/show_bug.cgi?id=1626687. I've noticed that uploading works for screenshots, but not for the photos, since they're generally greater in size.

Comment 1

[Ksenia Berezina [:ksenia]]

Hi :owlish and :snorp , wonder if you have any thoughts on this?

Comment 2

[[:owlish] 🦉 PST]

Ksenia, do you know if data urls are involved here? (i.e. if the upload happens through data url)

Comment 3

[Ksenia Berezina [:ksenia]]

:owlsih yeah, data urls are being used. The code is basically doing the following:

const img = document.createElement('img');
img.onload = () => {
 ...
}
img.onerror = () => {
 ...
}
img.src = "data:image/jpeg..."

I've attached a reduced test case with a data url for a 3.7MB image. It seems that in Firefox on Android img.onload or img.onerror doesn't get called. It works in Chrome on Android and in Firefox desktop. Hopefully this testcase is helpful (status should become loaded instead of pending).

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, it looks like we need to restrict the changes from Bug 1626687 to toplevel document URIs only. :owlish can you take care of that?

Comment 5

[[:owlish] 🦉 PST]

I'll take a look

Comment 8

[Vince Pike]

If there is a bug bounty to fix this on mobile I would gladly pay $ for it. I regularly donate to Mozilla every year, but this issue is a major one that I would like to see solved to keep using Firefox. Just let me know the amount.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1668952: Part 1 - Remove GeckoView-specific limits on URI length from Necko; r=#necko-reviewers

For webcompat reasons, we have determined that we should only limit the length
of URIs in specific cases. We're going to handle this on the GV side instead.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1668952: Part 2 - Fix handling of long toplevel data URIs in GeckoView; r=#geckoview-reviewers,agi!

While I evaluated multiple approaches to this problem, I arrived at handling
this directly from within Java, as this gives us better opportunities to report
the exact nature of the failure back to the embedding app.

We add a new error code specifically for this case.

In the case where no NavigationDelegate is registered, I used
GeckoSession.load throwing an exception as a crude way of notifying the app.

Depends on D109426

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1668952: Part 3 - update api.txt and changelog; r=#geckoview-reviewers

Depends on D109427

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1668952: Part 4 - Update GV tests for long URIs; r=#geckoview-reviewers

Now that we can report long URI errors to the app using NavigationDelegate,
I have updated the tests accordingly.

I also added a test to ensure that long data: URIs are still accepted when we
are not dealing with a toplevel document. We generate a PNG image filled with
randomness, which compresses poorly (and thus is "large") and yet is a valid PNG
file. We expect its onload event to fire.

Depends on D109428

Comment 13

[Vince Pike]

What does depends on D109428 mean? Does that mean that there is a fix for this problem to allow DataURLs greater than 2MB?
If so, any idea how I could test it? I would love to help with this being fixed, and like I said before I would be willing to pay for this.

Comment 14

[Vince Pike]

(In reply to Aaron Klotz [:aklotz] from comment #12)

Created attachment 9210872 [details]
Bug 1668952: Part 4 - Update GV tests for long URIs; r=#geckoview-reviewers

Now that we can report long URI errors to the app using NavigationDelegate,
I have updated the tests accordingly.

I also added a test to ensure that long data: URIs are still accepted when we
are not dealing with a toplevel document. We generate a PNG image filled with
randomness, which compresses poorly (and thus is "large") and yet is a valid PNG
file. We expect its onload event to fire.

Depends on D109428

I have updated my Firefox to the latest nightly, and still cannot get the large DataURLs to work correctly. Was that fixed at all, or are DataURLs still going to be limited to like 2MB ?

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

Until this issue is marked as Fixed and a comment is made with a link to the source code checkin like this example the bug will continue to exist. Once it is fixed it should show up in Geckoview Example in about 24h or less. For Fenix or other Geckoview based browsers you will need to wait until the Geckoview date code is greater than the date when this was checked in. Normally this is less than 24h but sometimes the upgrade automation breaks.

Comment 16

[Vince Pike]

(In reply to Kevin Brosnan [:kbrosnan] from comment #15)

Until this issue is marked as Fixed and a comment is made with a link to the source code checkin like this example the bug will continue to exist. Once it is fixed it should show up in Geckoview Example in about 24h or less. For Fenix or other Geckoview based browsers you will need to wait until the Geckoview date code is greater than the date when this was checked in. Normally this is less than 24h but sometimes the upgrade automation breaks.

Thank you very much for your helpful, and insightful answer!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd2b562ed225
Part 1 - Remove GeckoView-specific limits on URI length from Necko; r=necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/406fb35b61ca
Part 2 - Fix handling of long toplevel data URIs in GeckoView; r=geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/c6b68fe462d0
Part 3 - update api.txt and changelog; r=geckoview-reviewers,droeh
https://hg.mozilla.org/integration/autoland/rev/7bc2dd06085f
Part 4 - Update GV tests for long URIs; r=geckoview-reviewers,droeh

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/dd2b562ed225
https://hg.mozilla.org/mozilla-central/rev/406fb35b61ca
https://hg.mozilla.org/mozilla-central/rev/c6b68fe462d0
https://hg.mozilla.org/mozilla-central/rev/7bc2dd06085f

Comment 19

[Vince Pike]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #18)

https://hg.mozilla.org/mozilla-central/rev/dd2b562ed225
https://hg.mozilla.org/mozilla-central/rev/406fb35b61ca
https://hg.mozilla.org/mozilla-central/rev/c6b68fe462d0
https://hg.mozilla.org/mozilla-central/rev/7bc2dd06085f

Does this allow larger than 2MB data url's? Or does this just give an error message that it is occurring?

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

It does not allow large data URIs at the top level. At other levels, URIs of any length are allowed.

Comment 21

[Vince Pike]

(In reply to Aaron Klotz [:aklotz] from comment #20)

It does not allow large data URIs at the top level. At other levels, URIs of any length are allowed.

Thank you very much for your response, but I am still confused. What does top level mean? Does that mean like if I have a page that uses javascript to create an anchor element like the following that is 8MB, would that work now? (Currently it fails)
<a download='{filename}' id='File' href='data:application/octet-stream;base64,{base64_string}' target='_self'>Download</a>

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Vince Pike from comment #21)

(In reply to Aaron Klotz [:aklotz] from comment #20)

It does not allow large data URIs at the top level. At other levels, URIs of any length are allowed.

Thank you very much for your response, but I am still confused. What does top level mean? Does that mean like if I have a page that uses javascript to create an anchor element like the following that is 8MB, would that work now? (Currently it fails)
<a download='{filename}' id='File' href='data:application/octet-stream;base64,{base64_string}' target='_self'>Download</a>

Yes, that would fail. So would the user directly navigating to a long data URI.

But if the URI is for an iframe, or some other kind of content (in our test suite, I am using an img tag and set a long data: URI on that), then it succeeds.

Comment 23

[Vince Pike]

(In reply to Aaron Klotz [:aklotz] from comment #22)

(In reply to Vince Pike from comment #21)

(In reply to Aaron Klotz [:aklotz] from comment #20)

It does not allow large data URIs at the top level. At other levels, URIs of any length are allowed.

Thank you very much for your response, but I am still confused. What does top level mean? Does that mean like if I have a page that uses javascript to create an anchor element like the following that is 8MB, would that work now? (Currently it fails)
<a download='{filename}' id='File' href='data:application/octet-stream;base64,{base64_string}' target='_self'>Download</a>

Yes, that would fail. So would the user directly navigating to a long data URI.

But if the URI is for an iframe, or some other kind of content (in our test suite, I am using an img tag and set a long data: URI on that), then it succeeds.

Could you add an exception for an <a> tag, like the <img> tag you are currently allowing? Or is that something that it is decided to not be allowed to occur?

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

The reason for the limit is that the URIs are so long that the app runs out of memory and crashes.

Comment 25

[Vince Pike]

(In reply to Aaron Klotz [:aklotz] from comment #24)

The reason for the limit is that the URIs are so long that the app runs out of memory and crashes.

Thank you again very much for your patience with me! Can you set the limit to something like 250MB? Or something that would still allow it to be usable as a small(ish) file? My phone has 12GB of RAM, which I understand may be a lot more than most, but I would think a reasonable limit could be set similar to how this current 2MB limit is currently set.

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

At this point we're starting to venture into discussion territory, so I would suggest coming over to our chatroom on Matrix, where you can give us a better idea about your use case.

Comment 27

[Vince Pike]

I posted on Matrix the following. Please let me know if there is anything I can do to get this working for my use case. I do not want to abandon support for Firefox, but the Chromium browsers are allowing this behavior to occur, and it is what my application needs.

My use case, as is others would be to be able to save a file as an anchor tag with the base64 dataURL to allow the item to be downloaded. I currently have an app which can only expose it's data as a base64 string which works in an anchor tag, but the database file can be larger than 2MB, possibly if the user has accumulated a lot of data! What I would like to do is have the base64 dataURL's be able to be used up to 250MB, or even greater if you can allow it. I understand it may crash the browser but I empty the div I insert it into after I create the download link. I have no other way to expose my data besides this, so it makes my application a read only program with Firefox.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Based on https://github.com/mozilla-mobile/fenix/issues/19279#issuecomment-828511625 I believe <a download tags won't get treated as top-level and should work. (At least, the test case I posted to that issue uses a ~3MB image in a base64-encoded data uri on an <a download tag, and that reportedly works with this fix).