Remove promptForSaveToFile in favor of promptForSaveToFileAsync

RESOLVED FIXED in mozilla38

Status

Core Graveyard
File Handling
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Paolo, Assigned: Ganesh Sahukari, Mentored)

Tracking

({addon-compat})

Trunk
mozilla38
addon-compat

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
For compatibility reasons, in the External Helper App Handler, we call promptForSaveToFile and expect an error to be generated, instead of calling promptForSaveToFileAsync directly.

Unfortunately, this error is now reported to the Console.

We should just call promptForSaveToFileAsync directly.
(Assignee)

Comment 1

3 years ago
i would like to work on this bug. Can you assign me the bug
(Reporter)

Comment 2

3 years ago
I've assigned the bug to you. Basically, we call a JavaScript function from C++ using an XPConnect (IDL) interface, and the error is reported when the JavaScript function raises an expected exception. But we don't need the call in the first place, because all the sync implementations that exist in our code or in add-ons can easily be converted to use the async interface.

The calling code is here:

https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2243-2247

You can use the command "./mach build binaries" to rebuild the C++ only, without waiting for a full build.

And then we need to remove our promptForSaveToFile implementations all around the place:

https://dxr.mozilla.org/mozilla-central/search?q=promptForSaveToFile

One thing that's currently missing is error handling: we might want to call Cancel(NS_BINDING_ABORTED) if promptForSaveToFileAsync isn't implemented or throws (both cases result in a failure code in rv).

Looking forward to seeing your patch! As always, let me know if you have any questions.
Assignee: nobody → sahukariganesh2
Mentor: paolo.mozmail@amadzone.org
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Created attachment 8551727 [details] [diff] [review]
bug_1114594.patch
Attachment #8551727 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8551727 [details] [diff] [review]
bug_1114594.patch

Review of attachment 8551727 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! There are just a few style-related minor changes required. I'd like to take a look at the revised version, please upload a new patch marking this one as obsolete, and request review again.

In the meantime I've already started a full tryserver build to see if the other changes are correct:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2627e026a60b

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +2240,5 @@
>    nsIFile* fileToUse;
>    nsRefPtr<nsExternalAppHandler> kungFuDeathGrip(this);
>    nsCOMPtr<nsIHelperAppLauncherDialog> dlg(mDialog);
> +
> +  // we need to use the async version -> nsIHelperAppLauncherDialog.promptForSaveToFileAsync.

I believe this comment can be removed now, the code should be self-explanatory.

@@ +2241,5 @@
>    nsRefPtr<nsExternalAppHandler> kungFuDeathGrip(this);
>    nsCOMPtr<nsIHelperAppLauncherDialog> dlg(mDialog);
> +
> +  // we need to use the async version -> nsIHelperAppLauncherDialog.promptForSaveToFileAsync.
> +  rv = mDialog->PromptForSaveToFileAsync(this, 

Please ensure there are no extra spaces at end of line.

@@ +2246,5 @@
> +                                         GetDialogParent(),
> +                                         aDefaultFile.get(),
> +                                         aFileExtension.get(),
> +                                         mForceSave);
> +  if (rv != NS_OK) {

NS_FAILED(rv)

@@ +2248,5 @@
> +                                         aFileExtension.get(),
> +                                         mForceSave);
> +  if (rv != NS_OK) {
> +    Cancel(NS_BINDING_ABORTED);
> +    return;

The return statement is correct, but looks odd at the end of the function. I know people will need to remember to add it back if new code is added, but let's just remove it for now.
Attachment #8551727 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 5

3 years ago
Created attachment 8551913 [details] [diff] [review]
New patch for bug 1114594 with minor changes

Try Build failed due to the following reason

>>uriloader/exthandler/nsExternalHelperAppService.cpp:2240:12: error: unused >>variable 'fileToUse' [-Werror=unused-variable]

Corrected the above error too in the patch.
Corrected the patch according to the comments mentioned.
Attachment #8551727 - Attachment is obsolete: true
Attachment #8551913 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 6

3 years ago
(In reply to :Paolo Amadini from comment #4)
> Comment on attachment 8551727 [details] [diff] [review]
> bug_1114594.patch
> 
> Review of attachment 8551727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! There are just a few style-related minor changes required. I'd
> like to take a look at the revised version, please upload a new patch
> marking this one as obsolete, and request review again.
> 
> In the meantime I've already started a full tryserver build to see if the
> other changes are correct:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2627e026a60b
> 
> ::: uriloader/exthandler/nsExternalHelperAppService.cpp
> @@ +2240,5 @@
> >    nsIFile* fileToUse;
> >    nsRefPtr<nsExternalAppHandler> kungFuDeathGrip(this);
> >    nsCOMPtr<nsIHelperAppLauncherDialog> dlg(mDialog);
> > +
> > +  // we need to use the async version -> nsIHelperAppLauncherDialog.promptForSaveToFileAsync.
> 
> I believe this comment can be removed now, the code should be
> self-explanatory.
> 
> @@ +2241,5 @@
> >    nsRefPtr<nsExternalAppHandler> kungFuDeathGrip(this);
> >    nsCOMPtr<nsIHelperAppLauncherDialog> dlg(mDialog);
> > +
> > +  // we need to use the async version -> nsIHelperAppLauncherDialog.promptForSaveToFileAsync.
> > +  rv = mDialog->PromptForSaveToFileAsync(this, 
> 
> Please ensure there are no extra spaces at end of line.
> 
> @@ +2246,5 @@
> > +                                         GetDialogParent(),
> > +                                         aDefaultFile.get(),
> > +                                         aFileExtension.get(),
> > +                                         mForceSave);
> > +  if (rv != NS_OK) {
> 
> NS_FAILED(rv)
> 
> @@ +2248,5 @@
> > +                                         aFileExtension.get(),
> > +                                         mForceSave);
> > +  if (rv != NS_OK) {
> > +    Cancel(NS_BINDING_ABORTED);
> > +    return;
> 
> The return statement is correct, but looks odd at the end of the function. I
> know people will need to remember to add it back if new code is added, but
> let's just remove it for now.

Hi Paolo,

Try build have failed. I have corrected(build failure due to a unused variable) it in the new patch.
Can you start a tryserver build
(Reporter)

Comment 7

3 years ago
Comment on attachment 8551913 [details] [diff] [review]
New patch for bug 1114594 with minor changes

Hi Ganesh, sorry for the delay! I've started a new tryserver build here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1856414a7a3
Attachment #8551913 - Flags: review?(paolo.mozmail) → review+
(Assignee)

Comment 8

3 years ago
Try build is successful except for 1 failure, 1 busted and 5 retries. I see bugs have been raised for those build issues.
(Reporter)

Comment 9

3 years ago
(In reply to Ganesh Sahukari from comment #8)
> Try build is successful except for 1 failure, 1 busted and 5 retries. I see
> bugs have been raised for those build issues.

I agree this is ready to land:

https://hg.mozilla.org/integration/fx-team/rev/d34e894a9ea2
(Assignee)

Comment 10

3 years ago
Hi Paolo,

Thanks for helping in fixing the bug.
Can you suggest some wiki pages/code blocks to get a better understanding into firefox internals.
Or any good/starting component to check.
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1791532&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=1791174&repo=fx-team
Flags: needinfo?(sahukariganesh2)
(Reporter)

Comment 12

3 years ago
It's curious that the patch had failures on the fx-team tree, but those did not show up in the tryserver. I've started a new try build on top of a more recent base revision to see if the problem still occurs:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b121109daa2c
(Reporter)

Comment 13

3 years ago
(In reply to Ganesh Sahukari from comment #10)
> Thanks for helping in fixing the bug.

Thanks to you for fixing it :-) The error message definitely caused some confusion, and it's good to see it go away. Let's see if we can make sense of the test failures now!

> Can you suggest some wiki pages/code blocks to get a better understanding
> into firefox internals.
> Or any good/starting component to check.

There's a lot to choose from! I work mostly in the parts related to downloads, that would be the "toolkit/components/jsdownloads" folder for the back-end, and "browser/components/downloads/" for the front-end user interface. A good way to get familiar with the code, independently from the area, could be to search the Mozilla DXR index for an English string in the UI, for example "Go To Download Page":

https://dxr.mozilla.org/mozilla-central/search?q=%22Go+To+Download+Page%22

And from there follow the code through cmd.goToDownloadPage.label, and so on, searching MDN or asking questions for any Mozilla-specific reference you need.

Speaking of more specific help we need, I think for example of bug 1022816. It can take some more time than the ones you've worked on, but would be a good option if you can develop and test on Windows. It will allow us to fix a bug with modifications to temporary files being lost (see the downloads bug blocked by it).

There are a few other things we'd like to do in the back-end API:

https://bugzilla.mozilla.org/showdependencytree.cgi?id=825588&hide_resolved=1

And if you look at the front-end code for the panel, you'll see that most of it can definitely be simplified. I'm working on this already, but some extra help would be welcome and a good way to get familiar with the front-end.

Hope this helps!
https://hg.mozilla.org/projects/cypress/rev/1b7345f206df
(Assignee)

Comment 15

3 years ago
I have rerun the two failed tests manually on my local machine and i see no failures.

>>./mach xpcshell-test toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js
>>
>>Summary
>>=======
>>
>>Ran 539 tests (1 parents, 538 subtests)
>>Expected results: 539
>>Unexpected results: 0
>>
>>OK


>>./mach mochitest-plain security/manager/ssl/tests/mochitest/mixedcontent/test_bug383369.html
>>
>>10 INFO Passed:  2
>>11 INFO Failed:  0
>>12 INFO Todo:    0
>>13 INFO Slowest: 3312ms - /tests/security/manager/ssl/tests/mochitest/mixedcontent/test_bug383369.html
>>14 INFO SimpleTest FINISHED
>>15 INFO TEST-INFO | Ran 1 Loops
>>16 INFO SimpleTest FINISHED

But from the logs of failed tests which are reported, i see both of them have failed at the same code point

nsExternalAppHandler::Cancel() in uriloader/exthandler/nsExternalHelperAppService.cpp

From previous patch, we have added the extra NS_FAILED(rv) check,

>>rv = mDialog->PromptForSaveToFileAsync(this,
>>                                         GetDialogParent(),
>>                                         aDefaultFile.get(),
>>                                         aFileExtension.get(),
>>                                         mForceSave);
>>  if (NS_FAILED(rv)) {
>>    Cancel(NS_BINDING_ABORTED);
>>  }

But PromptForSaveToFileAsync on failure, calls the SaveDestinationAvailable() with null argument.

>>nsresult nsExternalAppHandler::SaveDestinationAvailable(nsIFile * aFile)
>>{
>>  if (aFile)
>>    ContinueSave(aFile);
>>  else
>>    Cancel(NS_BINDING_ABORTED);
>>
>>  return NS_OK;
>>}

which already takes care of calling Cancel(SaveDestinationAvailable) on failure.
and PromptForSaveToFileAsync() doesn't return anyvalue, or it is a void function. 

I will create a new patch taking care of above problem.
Flags: needinfo?(sahukariganesh2)
(Assignee)

Comment 16

3 years ago
Created attachment 8555160 [details] [diff] [review]
Made changes in nsExternalAppHandler::RequestSaveDestination()

Made changes to care of the mistake described in comment 15
Attachment #8555160 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 17

3 years ago
Comment on attachment 8555160 [details] [diff] [review]
Made changes in nsExternalAppHandler::RequestSaveDestination()

Review of attachment 8555160 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Ganesh Sahukari from comment #15)
> From previous patch, we have added the extra NS_FAILED(rv) check,
> But PromptForSaveToFileAsync on failure, calls the
> SaveDestinationAvailable() with null argument.
> which already takes care of calling Cancel(SaveDestinationAvailable) on
> failure.

That's correct, this is what happens when the called function has the expected behavior. The new error check should handle the case where promptForSaveToFileAsync does not behave as expected, and for example throws an exception before being able to call SaveDestinationAvailable(null). In this case, "rv" would be a failure code.

It's possible we have a similar problem somewhere in our code that we didn't catch before, but strangely, the new tryserver build apparently succeeded for the most part:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b121109daa2c

I've just restarted the few orange jobs, and will try to re-land the original patch if they succeed as well. Maybe there was a glitch in the infrastructure. The other possibility is that something changed really recently on fx-team and wasn't caught by the tryserver build from mozilla-central.
Attachment #8555160 - Flags: review?(paolo.mozmail)
(Reporter)

Comment 18

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/dd417504346b
Backed out for mass-crashes.
https://hg.mozilla.org/integration/fx-team/rev/1e87184509fa

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1806454&repo=fx-team (and a lot more like it)
The reason being that you changed the nsIHelperAppLauncherDialog interface without bumping the UUID. As a result, the build system was clueless to the fact that something changed and needed rebuilding. In the future, |./mach update-uuids nsIHelperAppLauncherDialog| will avoid problems like these. Also, Try builds are always clobbers, so issues like these won't appear there.

Re-landed with a UUID change.
https://hg.mozilla.org/integration/fx-team/rev/a5099b806777
https://hg.mozilla.org/mozilla-central/rev/a5099b806777
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Reporter)

Comment 22

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #20)
> The reason being that you changed the nsIHelperAppLauncherDialog interface
> without bumping the UUID.

I'm supposed to know this by this time... they say you never stop to learn new things, I'd add that I never stop to forget what I should have learned :-)

Sorry for the delay!
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.