Closed Bug 1668952 Opened 7 months ago Closed 1 month ago

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

Categories

(GeckoView :: General, defect, P1)

All
Android

Tracking

(Webcompat Priority:?, firefox89 fixed)

RESOLVED FIXED
89 Branch
Webcompat Priority ?
Tracking Status
firefox89 --- fixed

People

(Reporter: ksenia, Assigned: aklotz)

References

(Regression, )

Details

(Whiteboard: [geckoview:m88][geckoview:m89])

Attachments

(5 files)

Attached file 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.

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

Flags: needinfo?(snorp)
Flags: needinfo?(bugzeeeeee)
Priority: -- → P2

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

Flags: needinfo?(bugzeeeeee) → needinfo?(kberezina)

: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).

Flags: needinfo?(kberezina)

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?

Flags: needinfo?(snorp) → needinfo?(bugzeeeeee)
Regressed by: 1626687

I'll take a look

Flags: needinfo?(bugzeeeeee)
Assignee: nobody → bugzeeeeee
Duplicate of this bug: 1671511
Whiteboard: [geckoview:m88]
Assignee: bugzeeeeee → nobody
Priority: P2 → P1
Rank: 6
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Whiteboard: [geckoview:m88] → [geckoview:m88][geckoview:m89]
Duplicate of this bug: 1697727

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.

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.

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

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

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.

(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!

Attachment #9210870 - Attachment description: Bug 1668952: Part 2 - Add handling of long toplevel data URIs to GeckoSession; r=#geckoview-reviewers → Bug 1668952: Part 2 - Fix handling of long toplevel data URIs in GeckoView; r=#geckoview-reviewers,agi!
Pushed by aklotz@mozilla.com:
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

(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?

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

(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>

(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.

(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?

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

(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.

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.

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.

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).

You need to log in before you can comment on or make changes to this bug.