Closed Bug 1073259 Opened 8 years ago Closed 8 years ago

Make SettingsManager calls with invalid windows throw better errors

Categories

(Core :: DOM: Device Interfaces, defect)

25 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: qdot, Assigned: gerard-majax)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 3 obsolete files)

As of bug 900551 landing, if mozSettings.createLock() is called after inner-window-destroyed, an exception is thrown due to sendAsyncMessage (called from the SettingsLock constructor) needing a window. The prior version of createLock was always guaranteed to succeed (and many calls expect it to, since they're made in a chained way, i.e. mozSettings.createLock().get(whatever), so the current one should too.

This should be fixable by moving the createLock message to be sent the first time SettingsLock.sendMessage is called. Then we can always pass back a lock, and fail all requests after if it has no window to send to.
Actually, there's a far more simple fix: If we don't have a window, just return. We already do this in _closeHelper, but it should happen in sendMessage. This will make chained calls to get/set fail catastrophically when something tries to create a DOMRequest, but that should possibly be handled by DOMRequestHelper.
Attachment #8495625 - Attachment description: Patch 1 (v1) - Print more descriptive errors when SettingsManager tries to make request with invalid window → Patch 2 (v1) - Print more descriptive errors when SettingsManager tries to make request with invalid window
These patches should make errors more obvious, but do not fix the fact that Settings queries can fail after inner-window-destroyed. Best we can do there is a message to gaia.
Summary: Calling CreateLock on Settings should never fail or throw an exception → Make SettingsManager calls with invalid windows throw better errors
Attachment #8495624 - Flags: review?(fabrice) → review+
Comment on attachment 8495625 [details] [diff] [review]
Patch 2 (v1) - Print more descriptive errors when SettingsManager tries to make request with invalid window

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

::: dom/settings/SettingsManager.js
@@ +111,5 @@
> +    // before sending, otherwise just ignore.
> +    if (!this._settingsManager._window) {
> +      Cu.reportError("SettingsManager window died, cannot run settings transaction.");
> +      Cu.reportError("SettingsMessage: " + aMessageName);
> +      Cu.reportError("SettingsData: " + JSON.stringify(aData));

This should probably be just one long string rather than 3 callse to reportError().
Attachment #8495625 - Flags: review?(bent.mozilla) → review+
Whiteboard: [systemsfe]
Yay ... attachment 8495625 [details] [diff] [review] may help in bug 1074379
Flags: needinfo?(jlorenzo)
Carrying r+ from :bent

Can we land this ?
Assignee: kyle → lissyx+mozillians
Attachment #8495625 - Attachment is obsolete: true
Attachment #8504146 - Flags: review+
Flags: needinfo?(anygregor)
(In reply to Alexandre LISSY :gerard-majax from comment #7)
> Created attachment 8504146 [details] [diff] [review]
> 0001-Bug-1073529-Print-more-descriptive-errors-when-Setti.patch
> 
> Carrying r+ from :bent
> 
> Can we land this ?

Sure, go ahead!
Flags: needinfo?(anygregor)
Keywords: checkin-needed
(In reply to Alexandre LISSY :gerard-majax from comment #6)
The issue from bug 1074379 is still present with that patch :S
Flags: needinfo?(jlorenzo)
Is there a Try run for this?
Keywords: checkin-needed
And do both patches need to land?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> Is there a Try run for this?

https://tbpl.mozilla.org/?tree=Try&rev=f98b982e8afb
https://tbpl.mozilla.org/?tree=Try&rev=513ebd39b774
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> And do both patches need to land?

I think so.
(In reply to Alexandre LISSY :gerard-majax from comment #12)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> > Is there a Try run for this?
> 
> https://tbpl.mozilla.org/?tree=Try&rev=f98b982e8afb
> https://tbpl.mozilla.org/?tree=Try&rev=513ebd39b774

Ryan, those are finished, it's mostly green but you may want to have a look to make sure :)
Flags: needinfo?(ryanvm)
Looks good to me. Please fix the bug number in the commit messages and request checkin.
Flags: needinfo?(ryanvm)
Oh, I missed this, thanks for catching.
Attachment #8495624 - Attachment is obsolete: true
Attachment #8504146 - Attachment is obsolete: true
Comment on attachment 8506872 [details] [diff] [review]
Print more descriptive errors when SettingsManager tried to make request with invalid window r=bent

Carrying r+ from :bent, fixing bug number in commit message.
Attachment #8506872 - Flags: review+
Comment on attachment 8506873 [details] [diff] [review]
Print more descriptive errors when DOMRequestHelper tries to create request with invalid window r=fabrice

Carrying r+ from :fabrice, fixing bug number in commit message.
Attachment #8506873 - Flags: review+
Everything should be good now, fixed the commit messages, thanks for catching this!
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/b73bb9d2c9e1
https://hg.mozilla.org/mozilla-central/rev/3bd9a78ed444
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.