865803 - Unable to save Scratchpads, Style Editor, about:memory, ... on OS X

Status: RESOLVED FIXED

(DevTools Graveyard :: Scratchpad, defect)

Description

[Rob Campbell [:rc] (:robcee)]

Tried saving a Scratchpad on Nightly and am getting:

--
[14:46:58.105] TypeError: fp.file is null @ chrome://browser/content/scratchpad.js:1001

"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130423 Firefox/23.0"

Comment 1

[Rob Campbell [:rc] (:robcee)]

hg blame on that line points to:

Bug 781973 - Use filepicker's open() instead of the obsolete show() in /browser. r=bbondy

Comment 2

[Rob Campbell [:rc] (:robcee)]

Also breaks the save feature in the Style Editor.

Comment 3

[Rob Campbell [:rc] (:robcee)]

also, about:memory

Comment 4

[Rob Campbell [:rc] (:robcee)]

Places dialog, Export Bookmarks.

Comment 5

[Rob Campbell [:rc] (:robcee)]

Sync Prefs, Save recovery key...

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sync/utils.js#177

Comment 6

[Rob Campbell [:rc] (:robcee)]

tested and bug not present on Fx19, 20.

Comment 7

[Rob Campbell [:rc] (:robcee)]

bug not present in aurora, fx22.

Comment 8

[Rob Campbell [:rc] (:robcee)]

a new suspicious change:

http://hg.mozilla.org/mozilla-central/filelog/6e51c05eee68/widget/cocoa/nsFilePicker.mm

Comment 9

[:Gijs (he/him)]

Attachment: Nullcheck theDir before converting to a URL

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Can we write a test for this?

Comment 11

[Myk Melez [:myk] [@mykmelez]]

This breaks addons that use the file picker too, like Fabrice's b2gremote addon.

Comment 12

[:Gijs (he/him)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> Can we write a test for this?

The filepicker API blocks until the user responds to the dialog. So, only if we have a test framework in which we can interact with the system's native dialogs. Do we? I am not aware of one. :-(

Comment 13

[André Reinald]

(In reply to Rob Campbell [:rc] (:robcee) from comment #8)
> a new suspicious change:
> 
> http://hg.mozilla.org/mozilla-central/filelog/6e51c05eee68/widget/cocoa/
> nsFilePicker.mm

When saving the ScratchPad, execution doesn't reach nsFilePicker::PutLocalFile, so the native file save dialog is not displayed.

By contrast, nsFilePicker::PutLocalFile is called by "Save Page As…" and performs as expected.

The call chain is probably different and likely short returns on the way to calling PutLocalFile. We have to investigate this.

(In reply to :Gijs Kruitbosch from comment #9)
> Created attachment 742459 [details] [diff] [review]
> Nullcheck theDir before converting to a URL

theDir is checked for null a few lines above, and we make sure it's assigned either @"" or @"/Applications" so it can't be null any more.

Comment 14

[André Reinald]

Well, sorry, theDir is checked for null only in GetLocalFiles. And indeed an exception is thrown in PutLocalFile at
[thePanel setDirectoryURL:[[NSURL alloc] initFileURLWithPath:theDir isDirectory:YES]] when theDir is null.

But why is theDir set when saving a page, and null when saving the scratchpad?

Comment 15

[:Gijs (he/him)]

(In reply to André Reinald from comment #14)
> Well, sorry, theDir is checked for null only in GetLocalFiles. And indeed an
> exception is thrown in PutLocalFile at
> [thePanel setDirectoryURL:[[NSURL alloc] initFileURLWithPath:theDir
> isDirectory:YES]] when theDir is null.
> 
> But why is theDir set when saving a page, and null when saving the
> scratchpad?

Because you're allowed to not set it and have it use the last-used directory (OS X keeps track of that), rather than a pre-set one.

Comment 16

[:Gijs (he/him)]

Checked in: https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe8ad85f359

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dbe8ad85f359